xof / django-pglocks Goto Github PK

Without tagged versions automatics like version crawler aren't able to detect new version releases. Also finding out which commit has created which released version makes it hard to follow the development.

Could you please tag any released version with a tag?

The following paragraph in the documentation is contradicting with itself:

shared (default False) – If True, a shared lock is taken. Any number of sessions can hold a shared lock; if another session attempts to take an exclusive lock, it will wait until all shared locks are released; if a session is holding a shared lock, it will block attempts to take a shared lock. If False (the default), an exclusive lock is taken.

The bold parts are the contradicting parts. How can any number of sessions hold a shared lock while holding a shared lock will block the attempts to take a shared lock?

Someone should not write code like this:

from django.db import transaction, IntegrityError
from django_pglocks import advisory_lock
with transaction.atomic():
  with advisory_lock("foo") as acquired:
    transaction.set_rollback(True)
    raise IntegrityError('foo')

Practically speaking, replace transaction.set_rollback(True) and the exception raised with code that produces any kind of database error.

Reason: https://docs.djangoproject.com/en/1.11/topics/db/transactions/#controlling-transactions-explicitly

If you attempt to run database queries before the rollback happens, Django will raise a TransactionManagementError. You may also encounter this behavior when an ORM-related signal handler raises an exception.

In my environment, I can watch this in motion:

In [5]: from django.db import transaction, IntegrityError
   ...: from django_pglocks import advisory_lock
   ...: with transaction.atomic():
   ...:   with advisory_lock("foo") as acquired:
   ...:     transaction.set_rollback(True)
   ...:     raise IntegrityError('foo')
   ...: 
---------------------------------------------------------------------------
TransactionManagementError                Traceback (most recent call last)
<ipython-input-5-219258098aed> in <module>()
      4   with advisory_lock("foo") as acquired:
      5     transaction.set_rollback(True)
----> 6     raise IntegrityError('foo')

/usr/lib64/python2.7/contextlib.pyc in __exit__(self, type, value, traceback)
     33                 value = type()
     34             try:
---> 35                 self.gen.throw(type, value, traceback)
     36                 raise RuntimeError("generator didn't stop after throw()")
     37             except StopIteration, exc:

/venv/awx/lib/python2.7/site-packages/django_pglocks/__init__.pyc in advisory_lock(lock_id, shared, wait, using)
     78 
     79             command = base % release_params
---> 80             cursor.execute(command)
     81 
     82         cursor.close()

/venv/awx/lib/python2.7/site-packages/django/db/backends/utils.pyc in execute(self, sql, params)
     77         start = time()
     78         try:
---> 79             return super(CursorDebugWrapper, self).execute(sql, params)
     80         finally:
     81             stop = time()

/venv/awx/lib/python2.7/site-packages/django/db/backends/utils.pyc in execute(self, sql, params)
     57 
     58     def execute(self, sql, params=None):
---> 59         self.db.validate_no_broken_transaction()
     60         with self.db.wrap_database_errors:
     61             if params is None:

/venv/awx/lib/python2.7/site-packages/django/db/backends/base/base.pyc in validate_no_broken_transaction(self)
    446         if self.needs_rollback:
    447             raise TransactionManagementError(
--> 448                 "An error occurred in the current transaction. You can't "
    449                 "execute queries until the end of the 'atomic' block.")
    450 

TransactionManagementError: An error occurred in the current transaction. You can't execute queries until the end of the 'atomic' block.

From my testing, the lock still gets released, I'm unclear on the exact reasons for this. Anyway, another problem is that the "foo" exception here gets lost.

Problem: It's hard for a programmer to know that they shouldn't do this.

I think that this library should do something to avoid these situations, possibilities that I can suggest:

• Raise an error on entering the context manager if a transaction is active, tell programmer to not do that
• If in a transaction, save the rollback state, manually perform transaction.set_rollback(True) then release the lock, then set the rollback state to its prior value
• Catch TransactionManagementError when releasing the lock, and then do nothing (?) so that the original exception can get re-raised

I think there may still be some valid concerns about not releasing the lock, so I could see a reasonable argument that the 3rd option is a non-starter.

PR #22 was merged, adding metadata so that caniusepython3 would recognize that this package is compatible, but PyPI doesn't know about it yet, so the tool still flags this package as unsupported. Since it's just a metadata change, it might not even rate a version change, but this at least needs to be published to PyPI to update the metadata.

Hello!,

I've seen the current version is 1.0.2 (with python3 support) and the pypi one is the 1.0.1, could you update pypi to the current version?

Thanks! 👍

it would be great to see what you are using this library for in the readme

https://www.python.org/dev/peps/pep-0632/#migration-advice

I'm getting an acquired value of '', i.e. an empty string. I'm guessing that the lock isn't really being acquired. Any idea why?

The first error I see on Python 3 is that long doesn't exist. (Which means that there aren't any syntax errors, which is great.)

postgres supports arrays, a query to get locks on multiple keys at once could look like

SELECT pg_advisory_lock(a) FROM unnest(ARRAY[1,2]) as a;

$ DJANGO_SETTINGS_MODULE=django_pglocks.test_settings PYTHONPATH=. django-admin.py test
Creating test database for alias 'default'...
F
======================================================================
FAIL: test_basic_lock (django_pglocks.tests.PgLocksTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../django-pglocks/django_pglocks/tests.py", line 30, in test_basic_lock
    self.assertIsNone(acquired)
AssertionError: True is not None

Shouldn't this test assert for True-ness rather that None-ness?

Current implementation assumes atomic usage within a single context manager.

More complex workflows might require separation of lock and release calls, for example:

1. Acquire lock
2. Allow user to proceed to an editing interface (i.e. time passes)
3. Finish editing
4. Release lock (or timeout)

This doesn't seem to be possible with the current implementation. How can it be adapted to allow for such a scenario?

You can not protect database operations inside the advisory_lock context if you are running under Django transaction Middleware. The advisory_lock context is still good if you want to protect concurrent access to other resources (we use it to avoid stampede on some external resources)

For example, this code is as good as not locked:

with advisory_lock(42):
    obj = MyObject.get(id=42)
    obj.accessed +=1
    obj.save()

If you do not commit inside the context, then with two workers you can get the following behavior:

• A takes lock
• B wait on lock
• A read obj, accessed = 0
• A write accessed = 1 (change not committed)
• A release lock
• B takes lock (this goes very fast since B was waiting for it)
• B read obj, accessed = 0 (A change still not committed : it takes time to build the response)
• A commits
• B writes accessed = 1
• B commits

We had a very ugly bug in production because we were on the wrong impression our databases transactions were protected inside the advisory_lock context.

We had to write own py2/3 compatible version of this, that I share with you here under MIT license https://gist.github.com/lionel-panhaleux/bd62ade43afe57e6f365 .

There are many MIT variations, so I would appreciate if you could put full text of MIT license into the LICENSE.txt.