Do something to prevent error releasing lock in broken transaction

[AlanCoding]

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.

[tadeoiiit]

Another option would be to implement support for pg_try_advisory_xact_lock (cf. https://vladmihalcea.com/how-do-postgresql-advisory-locks-work) which is meant to be used inside transaction.

[jimmy927]

I think you have a good point and use-case.

However this is the behaviour i want personally for my current use-case, but i must admit i didn't expect it to work like this. I expected to catch some of the possible Exceptions and release the lock manually.