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
a845da8b0fc5bb172e278c399a1de9a2e49d62af)
--- /dev/null
+.. change::
+ :tags: bug, orm
+ :tickets: 7388
+
+ 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.
+
def _transaction_is_closed(self):
return not self._deactivated_from_connection
+ def _rollback_can_be_called(self):
+ # for RootTransaction / NestedTransaction, it's safe to call
+ # rollback() even if the transaction is deactive and no warnings
+ # will be emitted. tested in
+ # test_transaction.py -> test_no_rollback_in_deactive(?:_savepoint)?
+ return True
+
class MarkerTransaction(Transaction):
"""A 'marker' transaction that is used for nested begin() calls.
def _transaction_is_closed(self):
raise NotImplementedError()
+ def _rollback_can_be_called(self):
+ """indicates the object is in a state that is known to be acceptable
+ for rollback() to be called.
+
+ This does not necessarily mean rollback() will succeed or not raise
+ an error, just that there is currently no state detected that indicates
+ rollback() would fail or emit warnings.
+
+ It also does not mean that there's a transaction in progress, as
+ it is usually safe to call rollback() even if no transaction is
+ present.
+
+ .. versionadded:: 1.4.28
+
+ """
+ raise NotImplementedError()
+
def _get_subject(self):
raise NotImplementedError()
self.commit()
except:
with util.safe_reraise():
- self.rollback()
+ if self._rollback_can_be_called():
+ self.rollback()
finally:
if not out_of_band_exit:
subject._trans_context_manager = self._outer_trans_ctx
if not self._transaction_is_closed():
self.close()
else:
- self.rollback()
+ if self._rollback_can_be_called():
+ self.rollback()
finally:
if not out_of_band_exit:
subject._trans_context_manager = self._outer_trans_ctx
def _transaction_is_closed(self):
return self._state is CLOSED
+ def _rollback_can_be_called(self):
+ return self._state not in (COMMITTED, CLOSED)
+
class Session(_SessionClassMethods):
"""Manages persistence operations for ORM-mapped objects.
result = connection.exec_driver_sql("select * from users")
assert len(result.fetchall()) == 0
+ @testing.requires.independent_connections
+ def test_no_rollback_in_deactive(self, local_connection):
+ """test #7388"""
+
+ def fail(*arg, **kw):
+ raise BaseException("some base exception")
+
+ with mock.patch.object(testing.db.dialect, "do_commit", fail):
+ with expect_raises_message(BaseException, "some base exception"):
+ with local_connection.begin():
+ pass
+
+ @testing.requires.independent_connections
+ @testing.requires.savepoints
+ def test_no_rollback_in_deactive_savepoint(self, local_connection):
+ """test #7388"""
+
+ def fail(*arg, **kw):
+ raise BaseException("some base exception")
+
+ with mock.patch.object(
+ testing.db.dialect, "do_release_savepoint", fail
+ ):
+ with local_connection.begin():
+ with expect_raises_message(
+ BaseException, "some base exception"
+ ):
+ with local_connection.begin_nested():
+ pass
+
@testing.requires.savepoints
def test_nested_subtransaction_rollback(self, local_connection):
connection = local_connection
assert conn.closed
assert not fairy.is_valid
+ @testing.requires.independent_connections
+ def test_no_rollback_in_committed_state(self):
+ """test #7388
+
+ Prior to the fix, using the session.begin() context manager
+ would produce the error "This session is in 'committed' state; no
+ further SQL can be emitted ", when it attempted to call .rollback()
+ if the connection.close() operation failed inside of session.commit().
+
+ While the real exception was chained inside, this still proved to
+ be misleading so we now skip the rollback() in this specific case
+ and allow the original error to be raised.
+
+ """
+
+ sess = fixture_session()
+
+ def fail(*arg, **kw):
+ raise BaseException("some base exception")
+
+ with mock.patch.object(
+ testing.db.dialect, "do_rollback", side_effect=fail
+ ) as fail_mock, mock.patch.object(
+ testing.db.dialect,
+ "do_commit",
+ side_effect=testing.db.dialect.do_commit,
+ ) as succeed_mock:
+
+ # sess.begin() -> commit(). why would do_rollback() be called?
+ # because of connection pool finalize_fairy *after* the commit.
+ # this will cause the conn.close() in session.commit() to fail,
+ # but after the DB commit succeeded.
+ with expect_raises_message(BaseException, "some base exception"):
+ with sess.begin():
+ conn = sess.connection()
+ fairy_conn = conn.connection
+
+ eq_(succeed_mock.mock_calls, [mock.call(fairy_conn)])
+ eq_(fail_mock.mock_calls, [mock.call(fairy_conn)])
+
def test_continue_flushing_on_commit(self):
"""test that post-flush actions get flushed also if
we're in commit()"""
projects / thirdparty / sqlalchemy / sqlalchemy.git / commitdiff
