Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#21239 closed Bug (fixed)

Closing connection inside atomic block breaks atomicity

Reported by: marcin@… Owned by: Aymeric Augustin
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: atomic close
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Change History (10)

comment:1 by Aymeric Augustin, 10 years ago

Owner: changed from nobody to Aymeric Augustin
Status: newassigned
Triage Stage: UnreviewedAccepted

comment:2 by Aymeric Augustin, 10 years ago

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.

comment:3 by Anssi Kääriäinen, 10 years ago

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

comment:4 by Aymeric Augustin, 10 years ago

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.

comment:5 by Anssi Kääriäinen, 10 years ago

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.

comment:6 by Aymeric Augustin, 10 years ago

Severity: Release blockerNormal

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.

comment:8 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

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.

comment:9 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

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.

comment:10 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

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.

Note: See TracTickets for help on using tickets.
Back to Top