[Django] #22802: update Atomic class to make use of public transaction module-level API functions

Django

unread,

Jun 10, 2014, 2:31:40 PM6/10/14

to django-...@googlegroups.com

#22802: update Atomic class to make use of public transaction module-level API
functions
-------------------------------------+-------------------------------------
Reporter: gwilson | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database | Keywords:
layer (models, ORM) | Has patch: 0
Severity: Normal | Needs tests: 0
Triage Stage: | Easy pickings: 0
Unreviewed |
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
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 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/22802>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,

Jun 10, 2014, 2:35:25 PM6/10/14

to django-...@googlegroups.com

#22802: update Atomic class to make use of public transaction module-level API
functions
-------------------------------------+-------------------------------------
Reporter: gwilson | Owner: nobody
Type: | Status: new

* version: master => 1.6

--
Ticket URL: <https://code.djangoproject.com/ticket/22802#comment:1>

Django

unread,

Jun 10, 2014, 2:36:11 PM6/10/14

to django-...@googlegroups.com

#22802: update Atomic class to make use of public transaction module-level API
functions
-------------------------------------+-------------------------------------

Reporter: gwilson | Owner: gwilson
Type: | Status: assigned

* status: new => assigned
* owner: nobody => gwilson

--
Ticket URL: <https://code.djangoproject.com/ticket/22802#comment:2>

Django

unread,

Jun 10, 2014, 3:18:43 PM6/10/14

to django-...@googlegroups.com

#22802: update Atomic class to make use of public transaction module-level API
functions
-------------------------------------+-------------------------------------

Comment (by gwilson):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/22802#comment:3>

Django

unread,

Jun 10, 2014, 3:19:21 PM6/10/14

to django-...@googlegroups.com

#22802: update Atomic class to make use of public transaction module-level API
functions
-------------------------------------+-------------------------------------

Description changed by gwilson:

Old description:

> 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 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.

New description:

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.

--

--
Ticket URL: <https://code.djangoproject.com/ticket/22802#comment:4>

Django

unread,

Jun 10, 2014, 3:44:55 PM6/10/14

to django-...@googlegroups.com

#22802: update Atomic class to make use of public transaction module-level API
functions
-------------------------------------+-------------------------------------

Comment (by aaugustin):

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/22802#comment:5>

Django

unread,

Jun 10, 2014, 4:46:15 PM6/10/14

to django-...@googlegroups.com

#22802: update Atomic class to make use of public transaction module-level API
functions
-------------------------------------+-------------------------------------

Comment (by aaugustin):

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/22802#comment:6>

Django

unread,

Jun 10, 2014, 6:13:55 PM6/10/14

to django-...@googlegroups.com

#22802: update Atomic class to make use of public transaction module-level API
functions
-------------------------------------+-------------------------------------

Comment (by gwilson):

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/22802#comment:7>

Django

unread,

Jun 10, 2014, 8:34:54 PM6/10/14

to django-...@googlegroups.com

#22802: update Atomic class to make use of public transaction module-level API
functions
-------------------------------------+-------------------------------------

Has patch: 1 | Patch needs improvement: 1

Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by timo):

* needs_better_patch: 0 => 1
* has_patch: 0 => 1

Comment:

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

--
Ticket URL: <https://code.djangoproject.com/ticket/22802#comment:8>

Django

unread,

Jun 16, 2014, 4:19:10 PM6/16/14

to django-...@googlegroups.com

#22802: update Atomic class to make use of public transaction module-level API
functions
-------------------------------------+-------------------------------------

Reporter: gwilson | Owner: gwilson
Type: | Status: closed
Cleanup/optimization | Version: 1.6
Component: Database layer | Resolution: wontfix

Changes (by aaugustin):

* status: assigned => closed
* resolution: => wontfix

Comment:

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 [https://docs.djangoproject.com/en/dev/topics/db/sql
/#connections-and-cursors 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/22802#comment:9>

Reply all

Reply to author

Forward