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.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@8306 bcc190cf-cafb-0310-a4f2-bffc1f526a37

Author: jacobian

django/core/files/locks.py

@@ -40,20 +40,24 @@
 except (ImportError, AttributeError):
     pass
 
+def fd(f):
+    """Get a filedescriptor from something which could be a file or an fd."""
+    return hasattr(f, 'fileno') and f.fileno() or f
+
 if system_type == 'nt':
     def lock(file, flags):
-        hfile = win32file._get_osfhandle(file.fileno())
+        hfile = win32file._get_osfhandle(fd(file))
         win32file.LockFileEx(hfile, flags, 0, -0x10000, __overlapped)
 
     def unlock(file):
-        hfile = win32file._get_osfhandle(file.fileno())
+        hfile = win32file._get_osfhandle(fd(file))
         win32file.UnlockFileEx(hfile, 0, -0x10000, __overlapped)
 elif system_type == 'posix':
     def lock(file, flags):
-        fcntl.flock(file.fileno(), flags)
+        fcntl.flock(fd(file), flags)
 
     def unlock(file):
-        fcntl.flock(file.fileno(), fcntl.LOCK_UN)
+        fcntl.flock(fd(file), fcntl.LOCK_UN)
 else:
     # File locking is not supported.
     LOCK_EX = LOCK_SH = LOCK_NB = None

django/core/files/move.py

@@ -44,16 +44,17 @@ def file_move_safe(old_file_name, new_file_name, chunk_size = 1024*64, allow_ove
         pass
 
     # If the built-in didn't work, do it the hard way.
-    new_file = open(new_file_name, 'wb')
-    locks.lock(new_file, locks.LOCK_EX)
-    old_file = open(old_file_name, 'rb')
-    current_chunk = None
-
-    while current_chunk != '':
-        current_chunk = old_file.read(chunk_size)
-        new_file.write(current_chunk)
-
-    new_file.close()
-    old_file.close()
+    fd = os.open(new_file_name, os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, 'O_BINARY', 0))
+    try:
+        locks.lock(fd, locks.LOCK_EX)
+        old_file = open(old_file_name, 'rb')
+        current_chunk = None
+        while current_chunk != '':
+            current_chunk = old_file.read(chunk_size)
+            os.write(fd, current_chunk)
+    finally:
+        locks.unlock(fd)
+        os.close(fd)
+        old_file.close()
 
     os.remove(old_file_name)

django/core/files/storage.py

@@ -39,9 +39,9 @@ def save(self, name, content):
         # Get the proper name for the file, as it will actually be saved.
         if name is None:
             name = content.name
+        
         name = self.get_available_name(name)
-
-        self._save(name, content)
+        name = self._save(name, content)
 
         # Store filenames with forward slashes, even on Windows
         return force_unicode(name.replace('\\', '/'))
@@ -135,19 +135,41 @@ def _save(self, name, content):
             os.makedirs(directory)
         elif not os.path.isdir(directory):
             raise IOError("%s exists and is not a directory." % directory)
-            
-        if hasattr(content, 'temporary_file_path'):
-            # This file has a file path that we can move.
-            file_move_safe(content.temporary_file_path(), full_path)
-            content.close()
-        else:
-            # This is a normal uploadedfile that we can stream.
-            fp = open(full_path, 'wb')
-            locks.lock(fp, locks.LOCK_EX)
-            for chunk in content.chunks():
-                fp.write(chunk)
-            locks.unlock(fp)
-            fp.close()
+
+        # There's a potential race condition between get_available_name and
+        # saving the file; it's possible that two threads might return the
+        # same name, at which point all sorts of fun happens. So we need to
+        # try to create the file, but if it already exists we have to go back
+        # to get_available_name() and try again.
+
+        while True:
+            try:
+                # This file has a file path that we can move.
+                if hasattr(content, 'temporary_file_path'):
+                    file_move_safe(content.temporary_file_path(), full_path)
+                    content.close()
+
+                # This is a normal uploadedfile that we can stream.
+                else:
+                    # This fun binary flag incantation makes os.open throw an
+                    # OSError if the file already exists before we open it.
+                    fd = os.open(full_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, 'O_BINARY', 0))
+                    try:
+                        locks.lock(fd, locks.LOCK_EX)
+                        for chunk in content.chunks():
+                            os.write(fd, chunk)
+                    finally:
+                        locks.unlock(fd)
+                        os.close(fd)
+            except OSError:
+                # Ooops, we need a new file name.
+                name = self.get_available_name(name)
+                full_path = self.path(name)
+            else:
+                # OK, the file save worked. Break out of the loop.
+                break
+                
+        return name
 
     def delete(self, name):
         name = self.path(name)

tests/regressiontests/file_storage/tests.py

@@ -64,3 +64,38 @@
 >>> custom_storage.delete(first)
 >>> custom_storage.delete(second)
 """
+
+# Tests for a race condition on file saving (#4948).
+# This is written in such a way that it'll always pass on platforms 
+# without threading.
+
+import time
+from unittest import TestCase
+from django.core.files.base import ContentFile
+from models import temp_storage
+try:
+    import threading
+except ImportError:
+    import dummy_threading as threading
+
+class SlowFile(ContentFile):
+    def chunks(self):
+        time.sleep(1)
+        return super(ContentFile, self).chunks()
+
+class FileSaveRaceConditionTest(TestCase):
+    def setUp(self):
+        self.thread = threading.Thread(target=self.save_file, args=['conflict'])
+    
+    def save_file(self, name):
+        name = temp_storage.save(name, SlowFile("Data"))
+    
+    def test_race_condition(self):
+        self.thread.start()
+        name = self.save_file('conflict')
+        self.thread.join()
+        self.assert_(temp_storage.exists('conflict'))
+        self.assert_(temp_storage.exists('conflict_'))
+        temp_storage.delete('conflict')
+        temp_storage.delete('conflict_')
+