Modification to make annotation re-entrant and thread safe.

[ieb]

Apologies in advance if this suggestion is incorrect. I am seeing the _Transaction object shared between invocations in different requests, in different calls to the same method in the same request and between different threads making the same thread.

Doing the following appears to fix the problem for me. (DJango 1.3.1, Postgres 9.0.x, OSX and Ubuntu, Python 2.6)

You might want to drop the debugging.

'''python

from functools import wraps

from django.db import transaction, DEFAULT_DB_ALIAS, connections

import psycopg2.extensions
from django.http import HttpResponse
import logging

class _Transaction(object):
def init(self, using):
self.using = using
self.sid = None
self.complete = False
self.outer = True

def __enter__(self):
    if transaction.is_managed(self.using): 
        if connections[self.using].features.uses_savepoints:
            # We're already in a transaction; create a savepoint.
            logging.error("-ESP-------BEGIN---------------- %s " % self)
            self.sid = transaction.savepoint(self.using)
        self.outer = False
    else:
        logging.error("-EE-------BEGIN---------------- %s " % self)
        transaction.enter_transaction_management(using=self.using)
        transaction.managed(True, using=self.using)

def __exit__(self, exc_type, exc_value, traceback):
    if self.complete:
        return False
    if exc_value is None:
        # commit operation
        if self.outer and self.sid is None:
            # Outer transaction
            try:
                logging.error("-EE-------COMMIT---------------- %s " % self)
                transaction.commit(self.using)
            except:
                logging.error("-EE-------ROLLBACK-------------- %s " % self)
                transaction.rollback(self.using)
                raise
            finally:
                self._leave_transaction_management()
        elif self.sid is not None:
            # Inner savepoint
            try:
                logging.error("-ESP-------COMMIT---------------- %s " % self)
                transaction.savepoint_commit(self.sid, self.using)
            except:
                logging.error("-ESP-------ROLLBACK-------------- %s " % self)
                transaction.savepoint_rollback(self.sid, self.using)
                raise
    else:
        # rollback operation
        if self.outer and self.sid is None:
            # Outer transaction
            logging.error("-E--------ROLLBACK---------------- %s " % self)
            transaction.rollback(self.using)
            self._leave_transaction_management()
        elif self.sid is not None:
            # Inner savepoint
            logging.error("-E--------ROLLBACK-SAVE-POINT----- %s " % self)
            transaction.savepoint_rollback(self.sid, self.using)
    return False

def _leave_transaction_management(self):
    transaction.managed(False, using=self.using)
    transaction.leave_transaction_management(using=self.using)
    if not connections[self.using].is_managed() and connections[self.using].features.uses_autocommit:
        connections[self.using]._set_isolation_level(psycopg2.extensions.ISOLATION_LEVEL_AUTOCOMMIT)
        # Patch for bug in Django's psycopg2 backend; see:
        # https://code.djangoproject.com/ticket/16047

class _TransactionWrapper():

def __init__(self, using):
    self.using = using

def __call__(self, func):
    @wraps(func)
    def inner(*args, **kwargs):
        t = _Transaction(self.using)
        with t:
            o = func(*args, **kwargs)
            if hasattr(o, "status_code"):
                # If the response is a http response object and the status code indicates
                # any sort of failure, roll back either the savepoint or the whole
                # transaction
                if o.status_code < 200 or o.status_code >= 400:
                    if t.outer and t.sid is None: # outer transaction
                        logging.error("-R--------ROLLBACK---------------- %s " % t)
                        transaction.rollback(t.using)
                        t._leave_transaction_management()
                    elif t.sid is not None:
                        logging.error("-R--------ROLLBACK-SAVE-POINT----- %s " % t)
                        transaction.savepoint_rollback(t.sid, t.using)
                    t.complete = True
            return o
    return inner

def xact(using=None):
if using is None:
using = DEFAULT_DB_ALIAS
if callable(using):
return _TransactionWrapper(DEFAULT_DB_ALIAS)(using)
return _TransactionWrapper(using)

'''

[Xof]

Could you resubmit without the debugging output; it's making it hard to see what the intended change is. Thanks!

[Xof]

Definitely a bug, although splitting it that particular way breaks its ability to be used as a context manager. Working on a complete solution.

[kylemacfarlane]

I also had this issue a month or two ago and simply solved it by setting self.sid to an instance of local(). I could create a pull request for the modified version of xact I've been using in some projects.

[Xof]

Storing the sid in local() does indeed fix the multithreading problem, but not the reentrancy problem. (I'm not as concerned about reentrancy, but might as well kill both of them.)

[Xof]

Using transaction.is_managed(self.using) is a great idea; I'll add that to the next push I do.

[kylemacfarlane]

When would reentrancy even be an issue in a Django app?

Thread safety on the other hand is a major problem since the model managers are global. So if you use @xact on a manager method (a nice place for nested transactions) you wreck the DatabaseWrapper, and since DatabaseWrappers are also global you thus wreck your whole site and will suffer data loss even on views that don't use transactions. All together it creates a super charged version of Django ticket 16047.

[Xof]

The first reporter seemed to have a reentrancy issue. No argument on thread-safety!

[ieb]

I have a re-entrancy issue because I have a a web service (rest/json)
that acts as an aggregator for other services. Both the aggregator and
the other services need to have xact annotations and via the
aggregator the other services transactions are nested, and directly
they are not. I wanted the savepoint functionality as each aggregated
call is independent. Creating the inner _Transaction object on every
call ensures that its state is always correct for the circumstances in
which its used.

eg
@Xact
@restfull-endpoint
aggregator
calls service1, service2, service3

@Xact
@restfull-endpoint
service1

@Xact
@restfull-endpoint
service2

@Xact
@restfull-endpoint
service3

I dont have re-entrant in the sense of recursion (yet), so I was
probably using the term incorrectly.

On 18 May 2012 00:45, Christophe Pettus
reply@reply.github.com
wrote:

The first reporter seemed to have a reentrancy issue. No argument on thread-safety!

---

Reply to this email directly or view it on GitHub:
#1 (comment)

[kylemacfarlane]

Oh I see it. Since in the current code self.sid is not reset in __exit__ if you reuse the object it might not behave as expected.

test = _Transaction()
test.__enter__() # Begins transaction
test2 = _Transaction()
test2.__enter__() # Starts savepoint x1234
test2.__exit__() # Releases savepoint x1234
test.__exit__() # Commits transaction

# Now reuse test2
test2.__enter__() # Starts a transaction, but self.sid is still set to x1234...
test2.__exit__() # ERROR: Tries to release savepoint x1234 again instead of committing the transaction

But doesn't my pull request mitigate that problem by setting the self.sid every time it goes through __enter__, or am I still missing the whole problem? Now if you're also recursing with the same object then that's still another problem.

[Xof]

The right answer is to maintain the object on the stack. This happens correctly when used as a context manager, but decorators have their own issues; I have a fixed version in test now.