update Atomic class to make use of public transaction module-level API functions

[Gary Wilson]

Currently, the class operates using the connection object directly; however, this makes it more difficult for libraries that need to customize transaction management, e.g. ​https://github.com/jmoiron/johnny-cache (see also ​https://github.com/jmoiron/johnny-cache/issues/40).

Ideally, I believe that wrapping the entire transaction API into a class and making it configurable would be even better, as it would allow for transaction management customization without monkey patching. That said, I propose we at least make use of the public API functions for the 1.7 release, as that makes the transactions interface backwards-compatible with versions before the Atomic class landed.

[Gary Wilson]

​https://github.com/django/django/pull/2791

[Aymeric Augustin]

The main goal of my work on transactions in Django 1.6 was to provide strong guarantees of atomicity.

I'm uncomfortable with monkey-patching that has a non-negligible chance of breaking these guarantees.

[Aymeric Augustin]

That change also makes the implementation of Atomic a bit inconsistent.

It accesses some attributes directly on the connection object but performs an indirection through the module-level method for accessing methods.

[Gary Wilson]

Agree that the solution here is more pragmatic, rather than ideal. It does, in fact, make the Atomic class inconsistent in that it accesses both the module-level functions and the connection object directly. That said, it also makes the transaction code paths more consistent, while still ultimately making use of the methods on the connection object.

For a longer term solution, we should solidify a transaction API to use and make it possible customize the behavior/implementation without the need for third-party libraries to monkey-patch and/or recreate multiple connection class hierarchies.

Possible solutions may include:

• Maintain the transactions module, using either the existing functions or a new class. The latter would be desirable as it would be easy to then expose that as a database setting.
• Deprecate the functions in the transactions module, and instead make use of the DatabaseWrapper methods directly. The heart of the transaction management code lives in the DatabaseWrapper objects anyway; however, database settings currently only allow specifying the engine module, meaning that to swap out a customized DatabaseWrapper subclass would mean creating a custom engine package with a base.py module, etc. importing the original classes for everything but the custom DatabaseWrapper.
• Factoring out a base transaction management class that gets composed into the DatabaseWrapper instances. This is similar to the first item, except the idea here would be to actually contain the meat of the transaction management code instead of just a light wrapper around the connection object. This class, too, could then be something that could be swapped out with a database setting; however, they may still be complexities with class hierarchy should different database backends require custom transaction management classes.

That said, for a long-term solution, we should open a new ticket.

[Tim Graham]

The tests don't pass on Jenkins (see link on pull request) and segfault for me locally.

[Aymeric Augustin]

I checked the previous implementation. Indeed the old transaction decorators / context managers relied the module level functions. That was consistent because they were stateless (which was technically a bug).

The new atomic decorator / context manager is stateful. It stores its state on the connection object because, if several atomics are nested, they don't know about one another, they just share a connection. So the new implementation switched to using the connection everywhere for consistency.

I acknowledged that the DatabaseWrapper API is a bit scary, but transactions-related methods are well docstring'd. I don't belive they're much more difficult to patch than django.db.transactions. One just needs to inspect settings.DATABASES['...']['ENGINE'] to determine which classes need patching.

In addition, monkey-patching should really happen at that level. The docs say that ​connections and cursors do not follow PEP 249 for transaction handling, but connection.commit() or connection.rollback() work as expected and there's certainly some code that relies on this.

Considering how bad corruption at that level could get, I'm -1 on making changes to allow the johnny-cache authors to implement an unsafe solution.