Fix thread-related issues with extension data/media installation.
Review Request #6186 — Created July 31, 2014 and submitted
When attempting to install media/data for an extension, we'd grab a lock
so that only one thread/process on the system could write the files and
update the data at once. When originally written, this worked well
enough, as it was usingfcntl.flock()to lock the file. This wasn't
cross-platform, though, so we updated it to use Django's lock wrappers.Those wrappers are broken in Django versions prior to 1.7, for this
case. Instead of usingfcntl.flock(), they usefcntl.lockf(), which is
buggy on Python with threads, and results in separate threads being able
to grab the same lock at the same time, preventing the whole locking
strategy from working.This is fixed in newer versions of Django, but we can't wait for that.
Instead, we're pulling in thelocks.pyfrom Django 1.7 and sticking it
indjblets.util.compat. It's important to note that this module is not
guaranteed to remain indefinitely, as we would like to eventually use
Django's upstream version.There was also an issue where multiple threads all tried to delete the
lock file. A simple try/except around it fixes this. If the file wasn't
found, we ignore it, since another thread likely deleted it. Anything
else results in a log error. (This was the original problem that led me
down this rabbit hole.)To ensure that this all works, I've written a new unit test for media
installation. It makes use of a lot of the same multi-threaded testing
that another test used, so that has been pulled out into reusable
functions, simplifying both test cases considerably.Without these fixes, the new test catches the multiple calls for media
installation, and the breakage from attempting to delete the lock file
twice. With the fixes, the test passes.
Unit tests all pass. Without these fixes, the new test fails.
Change Summary:
Added the locks file from Django.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+209 -51) |
-
Tool: Pyflakes Processed Files: djblets/extensions/manager.py djblets/util/compat/django/core/files/locks.py djblets/extensions/tests.py Ignored Files: djblets/util/compat/django/core/files/__init__.py djblets/util/compat/__init__.py djblets/util/compat/django/__init__.py djblets/util/compat/django/core/__init__.py Tool: PEP8 Style Checker Processed Files: djblets/extensions/manager.py djblets/util/compat/django/core/files/locks.py djblets/extensions/tests.py Ignored Files: djblets/util/compat/django/core/files/__init__.py djblets/util/compat/__init__.py djblets/util/compat/django/__init__.py djblets/util/compat/django/core/__init__.py -
djblets/util/compat/django/core/files/locks.py (Diff revision 2) Col: 80 E501 line too long (89 > 79 characters)
-
-
djblets/util/compat/django/core/files/locks.py (Diff revision 2) Can you add a TODO comment to this saying that we can switch to the equivalent django implementation once we depend on 1.7+ ?
Change Summary:
Added a TODO.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+211 -51) |
-
Tool: Pyflakes Processed Files: djblets/extensions/manager.py djblets/util/compat/django/core/files/locks.py djblets/extensions/tests.py Ignored Files: djblets/util/compat/django/core/files/__init__.py djblets/util/compat/__init__.py djblets/util/compat/django/__init__.py djblets/util/compat/django/core/__init__.py Tool: PEP8 Style Checker Processed Files: djblets/extensions/manager.py djblets/util/compat/django/core/files/locks.py djblets/extensions/tests.py Ignored Files: djblets/util/compat/django/core/files/__init__.py djblets/util/compat/__init__.py djblets/util/compat/django/__init__.py djblets/util/compat/django/core/__init__.py -
djblets/util/compat/django/core/files/locks.py (Diff revision 3) Col: 80 E501 line too long (89 > 79 characters)
Tool: Pyflakes Processed Files: djblets/extensions/manager.py djblets/extensions/tests.py