Django

Code

Changeset 8306

Show
Ignore:
Timestamp:
08/11/08 11:51:18 (4 months ago)
Author:
jacob
Message:

Fixed #4948, a race condition in file saving. Thanks to Martin von Löwis, who diagnosed the problem and pointed the way to a fix.

Files:

Legend:

Unmodified
Added
Removed
Modified
Copied
Moved

  • django/trunk/django/core/files/locks.py

    r7814 r8306  
    4141    pass 
    4242 
     43def fd(f): 
     44    """Get a filedescriptor from something which could be a file or an fd.""" 
     45    return hasattr(f, 'fileno') and f.fileno() or f 
     46 
    4347if system_type == 'nt': 
    4448    def lock(file, flags): 
    45         hfile = win32file._get_osfhandle(file.fileno()) 
     49        hfile = win32file._get_osfhandle(fd(file)) 
    4650        win32file.LockFileEx(hfile, flags, 0, -0x10000, __overlapped) 
    4751 
    4852    def unlock(file): 
    49         hfile = win32file._get_osfhandle(file.fileno()) 
     53        hfile = win32file._get_osfhandle(fd(file)) 
    5054        win32file.UnlockFileEx(hfile, 0, -0x10000, __overlapped) 
    5155elif system_type == 'posix': 
    5256    def lock(file, flags): 
    53         fcntl.flock(file.fileno(), flags) 
     57        fcntl.flock(fd(file), flags) 
    5458 
    5559    def unlock(file): 
    56         fcntl.flock(file.fileno(), fcntl.LOCK_UN) 
     60        fcntl.flock(fd(file), fcntl.LOCK_UN) 
    5761else: 
    5862    # File locking is not supported. 

  • django/trunk/django/core/files/move.py

    r7814 r8306  
    4545 
    4646    # If the built-in didn't work, do it the hard way. 
    47     new_file = open(new_file_name, 'wb') 
    48     locks.lock(new_file, locks.LOCK_EX) 
    49     old_file = open(old_file_name, 'rb') 
    50     current_chunk = None 
    51  
    52     while current_chunk != '': 
    53         current_chunk = old_file.read(chunk_size) 
    54         new_file.write(current_chunk) 
    55  
    56     new_file.close() 
    57     old_file.close() 
     47    fd = os.open(new_file_name, os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, 'O_BINARY', 0)) 
     48    try: 
     49        locks.lock(fd, locks.LOCK_EX) 
     50        old_file = open(old_file_name, 'rb') 
     51        current_chunk = None 
     52        while current_chunk != '': 
     53            current_chunk = old_file.read(chunk_size) 
     54            os.write(fd, current_chunk) 
     55    finally: 
     56        locks.unlock(fd) 
     57        os.close(fd) 
     58        old_file.close() 
    5859 
    5960    os.remove(old_file_name) 

  • django/trunk/django/core/files/storage.py

    r8291 r8306  
    4040        if name is None: 
    4141            name = content.name 
     42         
    4243        name = self.get_available_name(name) 
    43  
    44         self._save(name, content) 
     44        name = self._save(name, content) 
    4545 
    4646        # Store filenames with forward slashes, even on Windows 
    …… 
    136136        elif not os.path.isdir(directory): 
    137137            raise IOError("%s exists and is not a directory." % directory) 
    138              
    139         if hasattr(content, 'temporary_file_path'): 
    140             # This file has a file path that we can move. 
    141             file_move_safe(content.temporary_file_path(), full_path) 
    142             content.close() 
    143         else: 
    144             # This is a normal uploadedfile that we can stream. 
    145             fp = open(full_path, 'wb') 
    146             locks.lock(fp, locks.LOCK_EX) 
    147             for chunk in content.chunks(): 
    148                 fp.write(chunk) 
    149             locks.unlock(fp) 
    150             fp.close() 
     138 
     139        # There's a potential race condition between get_available_name and 
     140        # saving the file; it's possible that two threads might return the 
     141        # same name, at which point all sorts of fun happens. So we need to 
     142        # try to create the file, but if it already exists we have to go back 
     143        # to get_available_name() and try again. 
     144 
     145        while True: 
     146            try: 
     147                # This file has a file path that we can move. 
     148                if hasattr(content, 'temporary_file_path'): 
     149                    file_move_safe(content.temporary_file_path(), full_path) 
     150                    content.close() 
     151 
     152                # This is a normal uploadedfile that we can stream. 
     153                else: 
     154                    # This fun binary flag incantation makes os.open throw an 
     155                    # OSError if the file already exists before we open it. 
     156                    fd = os.open(full_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, 'O_BINARY', 0)) 
     157                    try: 
     158                        locks.lock(fd, locks.LOCK_EX) 
     159                        for chunk in content.chunks(): 
     160                            os.write(fd, chunk) 
     161                    finally: 
     162                        locks.unlock(fd) 
     163                        os.close(fd) 
     164            except OSError: 
     165                # Ooops, we need a new file name. 
     166                name = self.get_available_name(name) 
     167                full_path = self.path(name) 
     168            else: 
     169                # OK, the file save worked. Break out of the loop. 
     170                break 
     171                 
     172        return name 
    151173 
    152174    def delete(self, name): 

  • django/trunk/tests/regressiontests/file_storage/tests.py

    r8244 r8306  
    6565>>> custom_storage.delete(second) 
    6666""" 
     67 
     68# Tests for a race condition on file saving (#4948). 
     69# This is written in such a way that it'll always pass on platforms  
     70# without threading. 
     71 
     72import time 
     73from unittest import TestCase 
     74from django.core.files.base import ContentFile 
     75from models import temp_storage 
     76try: 
     77    import threading 
     78except ImportError: 
     79    import dummy_threading as threading 
     80 
     81class SlowFile(ContentFile): 
     82    def chunks(self): 
     83        time.sleep(1) 
     84        return super(ContentFile, self).chunks() 
     85 
     86class FileSaveRaceConditionTest(TestCase): 
     87    def setUp(self): 
     88        self.thread = threading.Thread(target=self.save_file, args=['conflict']) 
     89     
     90    def save_file(self, name): 
     91        name = temp_storage.save(name, SlowFile("Data")) 
     92     
     93    def test_race_condition(self): 
     94        self.thread.start() 
     95        name = self.save_file('conflict') 
     96        self.thread.join() 
     97        self.assert_(temp_storage.exists('conflict')) 
     98        self.assert_(temp_storage.exists('conflict_')) 
     99        temp_storage.delete('conflict') 
     100        temp_storage.delete('conflict_') 
     101