Closing connection inside atomic block breaks atomicity

[marcin@…]

Closing the connection inside an atomic block rolls back queries up to that point, but doesn't prevent subsequent queries from reconnecting and getting committed. An exception should be raised in this situation in one of three places:

• connection.close()
• or on the next reconnection attempt
• or on the next query execution attempt

Testcase (currently failing):

def test_connection_closed_inside_atomic (self):
    with self.assertRaises(transaction.TransactionManagementError):
        with transaction.atomic():
            Reporter.objects.create(first_name="Tom")
            connection.close()
            Reporter.objects.create(first_name="Jerry")
        # here Jerry exists but Tom doesn't

[Aymeric Augustin]

I can't include a test to validate this behavior, because the test suite needs a constantly open connection when running with an in-memory SQLite database.

[Anssi Kääriäinen]

Can you skip the test on sqlite? It seems good enough that CI will catch errors here.

[Aymeric Augustin]

In the current code, I have included a comment that explains that I have chosen to allow calling close() within an atomic block to make it easy to dispose of a database connection regardless of its state.

At some point Django needs a method that kills the database connection regardless of its state. Even if this method isn't called close(), as soon as it's documented, it'll have this problem.

I see two ways to deal with this:

1) Document not to call close() within an atomic block. In practice, what's the use case for doing that?

2) Close the connection then raise an exception; however, this is going to create additional problems, because atomic will attempt to rollback to savepoints that don't exist anymore.

Currently I'm leaning towards option 1.

However, there's another ticket about what happens when the server closes the connection during a transaction, and it may require changes that will make it easy to support option 2.

[Anssi Kääriäinen]

Possible option 3) - if a connection is closed inside atomic() block, prevent queries until atomic block is exited. So, something like:

with atomic():
    with atomic():
        Model.objects.get(...)
        connection.close()
        Model.objects.get(...) # Raises exception about invalid transaction state
    Model.objects.get(...) # Still raises exception about invalid transaction state

I am *not* saying this is the way forward. This likely requires some complex code, and that doesn't seem worth it. If this happens to be easy (which would be a surprise), then it seems like a good candidate to consider.

[Aymeric Augustin]

Interesting idea. I'd really like to couple this fix with the "connection dropped" fix. Regardless of which side drops the connection Django's should most likely behave identically.

As agreed on IRC, we'll downgrade this to "normal" severity. I'll temporarily add a warning in the docs.

[Aymeric Augustin]

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

[Aymeric Augustin <aymeric.augustin@…>]

In 25860096f981b0b3026b38329e4b69c72b1d8db9:

Fixed #21239 -- Maintained atomicity when closing the connection.

Refs #15802 -- Reverted #7c657b24 as BaseDatabaseWrapper.close() now
has a proper "finally" clause that may need to preserve self.connection.

[Aymeric Augustin <aymeric.augustin@…>]

In 4ea02bdb0dc6776fc29d8164e417469c9ba10f18:

[1.6.x] Fixed #21239 -- Maintained atomicity when closing the connection.

Refs #15802 -- Reverted #7c657b24 as BaseDatabaseWrapper.close() now
has a proper "finally" clause that may need to preserve self.connection.

Backport of 25860096 from master.

[Aymeric Augustin <aymeric.augustin@…>]

In 2e42c859da42a871244aca36162a0aad2b63c02b:

[1.7.x] Fixed #21239 -- Maintained atomicity when closing the connection.

Refs #15802 -- Reverted #7c657b24 as BaseDatabaseWrapper.close() now
has a proper "finally" clause that may need to preserve self.connection.

Backport of 25860096 from master.