Skip to content

Deal with connection close (client- or server-initiated) in transaction.atomic() #2468

Merged
merged 4 commits into from Apr 10, 2014

4 participants

@aaugustin
Django member

See Trac tickets #21202 and #21239.

@shaib shaib commented on an outdated diff Mar 25, 2014
django/db/transaction.py
@@ -242,16 +247,26 @@ def __exit__(self, exc_type, exc_value, traceback):
# Roll back transaction
connection.rollback()
+ except Error:
+ # Error during rollback mean that the connection doesn't work any
+ # more. Maybe the database closed it. Clean up on our end.
+ connection.close()
@shaib
shaib added a note Mar 25, 2014

I think we can get here also from the raise in line 228 (that is, an error during commit, not rollback). I don't think closing is justified then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@akaariai
Django member

Could it be better to prevent all queries until outermost atomic block has been reached? That is, redefine validate_no_broken_transaction() to also prevent running queries when the connection has been closed and then reopened inside transaction. This seems to be the case when the server close the connection, as then when you execute a query you get an error, and that marks the transaction block as invalid.

aaugustin added some commits Mar 23, 2014
@aaugustin aaugustin Fixed #22321 -- Wrapped exceptions in _set_autocommit.
Refs #21202.
3becac8
@aaugustin aaugustin 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.
2586009
@aaugustin
Django member

@akaariai I think it keeps the code more clear not to mix these two behaviors.

Since the database connection is closed and not reopened, queries are prevented anyway.

aaugustin added some commits Mar 23, 2014
@aaugustin aaugustin Fixed #21202 -- Maintained atomicity when the server disconnects.
Thanks intgr for the report.

This commit doesn't include a test because I don't know how to emulate a
database disconnection in a cross-database compatible way.

Also simplified a 'backends' test that was constrained by this problem.
8176150
@aaugustin aaugustin Increased robustness of 58161e4e. Refs #22291. ee837b9
@aaugustin aaugustin merged commit ee837b9 into django:master Apr 10, 2014
@intgr

I think this comment might be slightly misleading, an error from rollback doesn't always mean a closed connection. But the coding seems prudent: we no longer know what state the transaction is in, so it's best to kill it. Much better than my patch.

Django member

Fair point, I'll update the comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.