Code

Opened 5 years ago

Last modified 12 months ago

#9433 new Bug

File locking broken on AFP mounts

Reported by: rndblnch Owned by: anonymous
Component: File uploads/storage Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Attachments (6)

locks.py.patch (861 bytes) - added by anonymous 5 years ago.
not_supported_locks.diff (893 bytes) - added by rndblnch 5 years ago.
better patch (adhere to <http://docs.djangoproject.com/en/dev/internals/contributing/> formatting policy)
inject_locks.patch (2.5 KB) - added by rndblnch 5 years ago.
makes the locks dependency explicit
inject_locks_Django-1.1.1.patch (2.6 KB) - added by rndblnch 4 years ago.
revised patch against django 1.1.1
inject_locks_Django-1.5a1.patch (2.7 KB) - added by rndblnch 17 months ago.
patch bumped to 1.5a1
inject_locks_Django-1.5a1.2.patch (2.7 KB) - added by rndblnch 17 months ago.
patch bumped to 1.5a1

Download all attachments as: .zip

Change History (22)

comment:1 follow-up: Changed 5 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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.

comment:2 in reply to: ↑ 1 Changed 5 years ago by anonymous

  • Has patch set

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.

Changed 5 years ago by anonymous

Changed 5 years ago by rndblnch

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

comment:3 follow-up: Changed 5 years ago by rndblnch

  • Owner changed from nobody to 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

comment:4 in reply to: ↑ 3 Changed 5 years ago by 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).

comment:5 Changed 5 years ago by 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.

  1. patch review wanted (ticket #9433)

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

Changed 5 years ago by rndblnch

makes the locks dependency explicit

comment:6 Changed 5 years ago by anonymous

  • Owner changed from rndblnch to anonymous

comment:7 Changed 5 years ago by rndblnch

  • Version changed from 1.0 to 1.1

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 :)

Changed 4 years ago by rndblnch

revised patch against django 1.1.1

comment:8 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:9 follow-up: Changed 2 years ago by lrekucki

  • Easy pickings unset
  • Patch needs improvement set
  • UI/UX unset

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.

comment:10 Changed 22 months ago by anonymous

i have the same problem!

comment:11 in reply to: ↑ 9 Changed 17 months ago by 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
>>> 

Changed 17 months ago by rndblnch

patch bumped to 1.5a1

Changed 17 months ago by rndblnch

patch bumped to 1.5a1

comment:12 Changed 17 months ago by 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.

comment:13 Changed 17 months ago by anonymous

  • Easy pickings set

comment:14 Changed 17 months ago by anonymous

  • Patch needs improvement unset
  • Version changed from 1.1 to 1.5-alpha-1

comment:15 follow-up: Changed 14 months ago by dokterbob

  • Easy pickings unset
  • Patch needs improvement set
  • Version changed from 1.5-alpha-1 to master

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

comment:16 in reply to: ↑ 15 Changed 12 months ago by 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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.