Opened 17 months ago

Closed 11 months ago

Last modified 11 months ago

#21239 closed Bug (fixed)

Closing connection inside atomic block breaks atomicity

Reported by: marcin@… Owned by: aaugustin
Component: Database layer (models, ORM) Version: master
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 Changed 17 months ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to aaugustin
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 17 months ago by aaugustin

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 Changed 17 months ago by akaariai

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

comment:4 Changed 17 months ago by aaugustin

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 Changed 17 months ago by akaariai

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 Changed 17 months ago by aaugustin

  • Severity changed from Release blocker to Normal

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 Changed 11 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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 Changed 11 months ago by 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.

comment:10 Changed 11 months ago by 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.

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