Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

try not to call .rollback() in ctx manager in committed state #7388

Closed
zzzeek opened this issue Dec 1, 2021 · 2 comments
Closed

try not to call .rollback() in ctx manager in committed state #7388

zzzeek opened this issue Dec 1, 2021 · 2 comments
Labels
bug Something isn't working orm
Milestone

Comments

@zzzeek
Copy link
Member

zzzeek commented Dec 1, 2021

the trace linked at #7286 shows the problem

in session.commit(), if session.close() raises an exception (connection return to pool failed because of mysql can't .rollback()), the context manager will do a rollback(), which then hits the assertion at

self._assert_active(prepared_ok=True, rollback_ok=True)

i can't think of a safe way to not actually call rollback() here so I think making a more clear exception is the best we can do. propose messaging change as follows:

diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index 8212a111d5..3bbd40bb54 100644
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -583,13 +583,26 @@ class SessionTransaction(TransactionalContext):
         prepared_ok=False,
         rollback_ok=False,
         deactive_ok=False,
+        is_rollback=False,
         closed_msg="This transaction is closed",
     ):
         if self._state is COMMITTED:
-            raise sa_exc.InvalidRequestError(
-                "This session is in 'committed' state; no further "
-                "SQL can be emitted within this transaction."
-            )
+            if is_rollback:
+                raise sa_exc.InvalidRequestError(
+                    "This session is in 'committed' state, indicating the "
+                    "SQL transaction has committed successfully, however the "
+                    "connectivity resources have not yet been successfully "
+                    "released; rollback() cannot proceed.  If this error is "
+                    "occurring within a context manager against begin() "
+                    "that calls rollback() automatically, examine the "
+                    "exception chain to determine the cause of the failure "
+                    "to release resources."
+                )
+            else:
+                raise sa_exc.InvalidRequestError(
+                    "This session is in 'committed' state; no further "
+                    "SQL can be emitted within this transaction."
+                )
         elif self._state is PREPARED:
             if not prepared_ok:
                 raise sa_exc.InvalidRequestError(
@@ -849,7 +862,7 @@ class SessionTransaction(TransactionalContext):
         return self._parent
 
     def rollback(self, _capture_exception=False, _to_root=False):
-        self._assert_active(prepared_ok=True, rollback_ok=True)
+        self._assert_active(prepared_ok=True, rollback_ok=True, is_rollback=True)
 
         stx = self.session._transaction
         if stx is not self:

to test this, need to mock out connection.close() such that it will throw an exception.

@zzzeek zzzeek added bug Something isn't working orm labels Dec 1, 2021
@zzzeek zzzeek added this to the 1.4.x milestone Dec 1, 2021
@zzzeek zzzeek changed the title guard against calling rollback() while in committed state message more clearly when rollback() is called within a failed .commit() Dec 1, 2021
@zzzeek zzzeek changed the title message more clearly when rollback() is called within a failed .commit() try not to call .rollback() in ctx manager in committed state Dec 2, 2021
@sqla-tester
Copy link
Collaborator

Mike Bayer has proposed a fix for this issue in the main branch:

contextmanager skips rollback if trans says to skip it https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3343

@sqla-tester
Copy link
Collaborator

Mike Bayer has proposed a fix for this issue in the rel_1_4 branch:

contextmanager skips rollback if trans says to skip it https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3344

sqlalchemy-bot pushed a commit that referenced this issue Dec 7, 2021
Fixed issue where if an exception occurred when the :class:`_orm.Session`
were to close the connection within the :meth:`_orm.Session.commit` method,
when using a context manager for :meth:`_orm.Session.begin` , it would
attempt a rollback which would not be possible as the :class:`_orm.Session`
was in between where the transaction is committed and the connection is
then to be returned to the pool, raising the exception "this
sessiontransaction is in the committed state". This exception can occur
mostly in an asyncio context where CancelledError can be raised.

Fixes: #7388
Change-Id: I1a85a3a7eae79f3553ddf1e3d245a0d90b0a2f40
(cherry picked from commit a845da8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working orm
Projects
None yet
Development

No branches or pull requests

2 participants