File locking broken on AFP mounts

[rndblnch]

File locking does not work on volumes mounted using AFP on Mac OS X 10.5

django/core/files/locks.py assumes that the presence of fcntl module implies a posix system and that for a posix system fcntl.lockf works for every file accessible on the file system.
This is not the case on a Mac OS X box for files on a volume mounted using AFP:

[taquet:~] iihm% uname -mprs
Darwin 9.5.0 Power Macintosh powerpc
[taquet:~] iihm% python
Python 2.5.1 (r251:54863, Jan 17 2008, 19:35:16) 
[GCC 4.0.1 (Apple Inc. build 5465)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import django
>>> django.VERSION
(1, 0, 'final')
>>> from django.core.files import locks
>>> f = open("/Volumes/Web/test.txt", "w")
>>> locks.lock(f, locks.LOCK_EX)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Library/Python/2.5/site-packages/django/core/files/locks.py", line 57, in lock
    fcntl.lockf(fd(file), flags)
IOError: [Errno 45] Operation not supported

A workaround is to revert [8675].
I know this is not acceptable since it fixes #8403, but it is still a fact that from a Mac OS X user perspective, [8675] introduce a regression.

[Malcolm Tredinnick]

We're not going to revert that change for one particular sort of filesystem on one particular operating system, since it fixes a much broader class of problems. That would be very counter-productive.

So the real solution here is coming up with a way to work out when that locking attempt won't work. Since we're using the recommended approach to file locking, file systems that don't support it will need specific workarounds (or a warning saying one can't use them for that purpose). Assuming POSIX-like behaviour is quite valid, but exceptional handling for edge-cases is reasonable if we can detect them.

You have one of these filesystems available. So can you look up how we can detect the problem? Does flock() work in those cases and, if so, is it possible to try lockf() and, if that fails, fall back to flock()? Do some experiments and see what you can come up with.

[anonymous]

Replying to mtredinnick:

We're not going to revert that change for one particular sort of filesystem on one particular operating system, since it fixes a much broader class of problems. That would be very counter-productive.

I understand that, that is why i said it is a workaround (not a fix).
I mentioned it because it can help other people before we find a proper fix.

So the real solution here is coming up with a way to work out when that locking attempt won't work. Since we're using the recommended approach to file locking, file systems that don't support it will need specific workarounds (or a warning saying one can't use them for that purpose). Assuming POSIX-like behaviour is quite valid, but exceptional handling for edge-cases is reasonable if we can detect them.

The problem is that the behaviour is not consistant on the whole filesystem: local volumes are ok, volumes mounted via AFP are not.
So special casing at import-time of django.core.files.locks id not possible.
I propose the following solution:
in the "posix" case, enclosing the calls to fcntl.lockf in a try block and in case of IOError for "Operation not supported" error, defaults silently to do nothing (which is the fallback already used when fcntl is not available.

I've attached a patch against trunk.

You have one of these filesystems available. So can you look up how we can detect the problem? Does flock() work in those cases and, if so, is it possible to try lockf() and, if that fails, fall back to flock()? Do some experiments and see what you can come up with.

Falling back to flock does not help, it's a do nothing operation on AFP volumes.

[rndblnch]

better patch (adhere to <​http://docs.djangoproject.com/en/dev/internals/contributing/> formatting policy)

[rndblnch]

ok, I've submitted a better patch against trunk that follow the "Contributing to Django" advice for formatting.
i'm volunteering to be assigned to this bug, and waiting the triagers/core developers to make a decision.
with kind regards,
renaud

[rndblnch]

for the record: this issue was previously reported here: #8403, and the proposed patch could be adapted easily to also fix #9400 which has the same origin (switching from flock to lockf).

[rndblnch]

after getting some feedback on the django-developers mailing list [0], i'm back with another proposed way for fixing this issue.

i understand that my case is special and that solving it according to the not_supported_locks.diff patch proposed above may not be optimal.
what i propose is to provide a way to replace the locks module if needed at user will.
in the new attached patch against trunk (inject_locks.patch), this is done by adding a locks parameter that defaults to django.core.files.locks to the class/functions that use it (i.e. django.core.files.move.file_move_safe and django.core.files.storage.FileSystemStorage).

it is then possible to fix the bug from the application code by defining a patched_locks.py module like this one:

from django.core.files import locks

def ignore(*args): pass

def patch(f, Error=IOError, errno=45, fallback=ignore):
	def g(*args):
		try:
			f(*args)
		except Error, e:
			if e.errno == errno:
				fallback(*args)
			else:
				raise
	g.__name__ = f.__name__
	g.__doc__  = g.__doc__
	return g			
			
lock   = patch(locks.lock)
unlock = patch(locks.unlock)

LOCK_EX = locks.LOCK_EX
LOCK_SH = locks.LOCK_SH
LOCK_NB = locks.LOCK_NB

and in the models.py code that use a FileField:

from django.db import models
from django.core.files.storage import FileSystemStorage
import patched_locks

class Test(models.Model):
	document = models.FileField(storage=FileSystemStorage(locks=patched_locks))

doing so will also allow to make the FileSystemStorage class easier to test.
hope this solution helps.

0. patch review wanted (ticket #9433)

<​http://groups.google.com/group/django-developers/browse_thread/thread/e53852d7ec79bbd5#>

[rndblnch]

makes the locks dependency explicit

[rndblnch]

the problem still affects 1.1, however, the inject_locks.patch is still relevant and makes it possible to cleanly pass another locks implementation at user will.
hope it will get some review and feedback :)

[rndblnch]

revised patch against django 1.1.1

[Łukasz Rekucki]

The patch doesn't apply (needs a very simple cleanup), but more importantly this needs checking if it's still a problem on OS X 10.7, etc.

[anonymous]

i have the same problem!

[rndblnch]

Replying to lrekucki:

The patch doesn't apply (needs a very simple cleanup)

i will do that

but more importantly this needs checking if it's still a problem on OS X 10.7, etc.

yes it is, see the example below where /tmp is local and /Volumes/blanch/temp is distant volume (from a MacOSX 10.8 box) mounted via afp on the local box (also running 10.8)

[esperluet:Shared/src/Django-1.5a1] blanch% python
Python 2.7.2 (default, Jun 20 2012, 16:23:33) 
[GCC 4.2.1 Compatible Apple Clang 4.0 (tags/Apple/clang-418.0.60)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import django
>>> django.get_version()
u'1.5a1'
>>> from django.core.files import locks
>>> f = open("/tmp/test.txt", "w") # local file
>>> locks.lock(f, locks.LOCK_EX)
>>> f = open("/Volumes/blanch/temp/test.txt", "w") # distant file mounted via afp
>>> locks.lock(f, locks.LOCK_EX)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "django/core/files/locks.py", line 56, in lock
    fcntl.lockf(fd(file), flags)
IOError: [Errno 45] Operation not supported
>>> 

[rndblnch]

patch bumped to 1.5a1

[rndblnch]

patch bumped to 1.5a1

[anonymous]

patch adapted to apply on top of 1.5a1.
i have reloaded the attachement apge while uploading, hence the double attachment, but it is the same file.

[Mathijs de Bruin]

The problem is confirmed to (still) exist in the master branch (e369dc) using the test code above, on OS X 10.6.8.

However, the error is thrown regardless of whether the patch is applied or not.

A fork with the patch applied lives on my GitHub account: ​https://github.com/dokterbob/django/tree/ticket_9433

[rndblnch]

Replying to dokterbob:

The problem is confirmed to (still) exist in the master branch (e369dc) using the test code above, on OS X 10.6.8.

However, the error is thrown regardless of whether the patch is applied or not.

that is because the patch does not fix the bug per it-self.
it just allows the client code to provide an alternative implementation to django.core.files.locks which is used by default by the django.core.files.storage module.
fixing locks it-self for AFP volumes was not considered an option at the time (4 years ago).

see comment:5 for a summary of the discussion + how to fix (read "ignore") the bug from client code once the patch is applied.

A fork with the patch applied lives on my GitHub account: ​https://github.com/dokterbob/django/tree/ticket_9433

[Mariusz Felisiak]

fcntl.lockf() was changed to fcntl.flock() in 6fe26bd3ee75a6d804ca3861181ad61b1cca45ea.