Skip to content

future engine does not close connection on failed begin #7272

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.

Sign up for GitHub

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

Jump to bottom
Closed
zzzeek opened this issue Nov 1, 2021 · 5 comments
Closed

future engine does not close connection on failed begin #7272

zzzeek opened this issue Nov 1, 2021 · 5 comments
Labels
bug Something isn't working engine engines, connections, transactions, isolation levels, execution options
Milestone
1.4.x

Comments

@zzzeek
Copy link
Member

zzzeek commented Nov 1, 2021 •
edited
Loading

this test didnt get set up in test_execute

diff --git a/test/engine/test_execute.py b/test/engine/test_execute.py
index 91cc60fc7b..327dd4cc06 100644
--- a/test/engine/test_execute.py
+++ b/test/engine/test_execute.py
@@ -815,8 +815,10 @@ class ConvenienceExecuteTest(fixtures.TablesTest):
         mock_connection = Mock(
             return_value=Mock(begin=Mock(side_effect=Exception("boom")))
         )
-        engine._connection_cls = mock_connection
-        assert_raises(Exception, engine.begin)
+        with mock.patch.object(engine, "_connection_cls", mock_connection):
+            with expect_raises_message(Exception, "boom"):
+                with engine.begin() as conn:
+                    pass
 
         eq_(mock_connection.return_value.close.mock_calls, [call()])
 
@@ -862,6 +864,7 @@ class ConvenienceExecuteTest(fixtures.TablesTest):
             fn(conn, 5, value=8)
         self._assert_fn(5, value=8)
 
+    @testing.requires.legacy_engine
     def test_connect_as_ctx_noautocommit(self):
         fn = self._trans_fn()
         self._assert_no_data()
@@ -872,6 +875,9 @@ class ConvenienceExecuteTest(fixtures.TablesTest):
             # autocommit is off
             self._assert_no_data()
 
+class FutureConvenienceExecuteTest(fixtures.FutureEngineMixin, ConvenienceExecuteTest):
+    __backend__ = True
+
 
 class CompiledCacheTest(fixtures.TestBase):
     __backend__ = True

if begin fails, the connection stays opened, fix:

diff --git a/lib/sqlalchemy/future/engine.py b/lib/sqlalchemy/future/engine.py
index ab890ca4f4..06eecb7dd4 100644
--- a/lib/sqlalchemy/future/engine.py
+++ b/lib/sqlalchemy/future/engine.py
@@ -358,7 +358,11 @@ class Engine(_LegacyEngine):
             self.conn = conn
 
         def __enter__(self):
-            self.transaction = self.conn.begin()
+            try:
+                self.transaction = self.conn.begin()
+            except:
+                self.conn.close()
+                raise
             self.transaction.__enter__()
             return self.conn
@zzzeek zzzeek added bug Something isn't working engine engines, connections, transactions, isolation levels, execution options labels Nov 1, 2021
@zzzeek zzzeek added this to the 1.4.x milestone Nov 1, 2021
@zzzeek
Copy link
Member Author

zzzeek commented Nov 1, 2021

although at the same time the future context manager is acting like the asyncio one where it doesnt begin until you enter it. this is also wrong for a sync context manager. begin() should begin() the transaction, just like the old one.

@zzzeek
Copy link
Member Author

zzzeek commented Nov 1, 2021

the sessionmaker().begin() context manager does not evaluate the begin() until entered. need to see if there is some precedent on this.

@zzzeek
Copy link
Member Author

zzzeek commented Nov 1, 2021 •
edited
Loading

here's the official "begin a transaction" example in pep 343:

from contextlib import contextmanager
from unittest import mock

@contextmanager
def transaction(db):
    db.begin()
    try:
        yield None
    except:
        db.rollback()
        raise
    else:
        db.commit()


m1 = mock.Mock()

transaction(m1)
assert m1.mock_calls == []


with transaction(m1):
    pass

assert m1.mock_calls == [mock.call.begin(), mock.call.commit()]

above, db.begin() is not called until the context manager is entered.

@sqla-tester
Copy link
Collaborator

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

use full context manager flow for future.Engine.begin() https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3252

@sqla-tester
Copy link
Collaborator

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

use full context manager flow for future.Engine.begin() https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3253

sqlalchemy-bot pushed a commit that referenced this issue Nov 1, 2021
@zzzeek
use full context manager flow for future.Engine.begin()
370624c 
Fixed issue in future :class:`_future.Engine` where calling upon
:meth:`_future.Engine.begin` and entering the context manager would not
close the connection if the actual BEGIN operation failed for some reason,
such as an event handler raising an exception; this use case failed to be
tested for the future version of the engine. Note that the "future" context
managers which handle ``begin()`` blocks in Core and ORM don't actually run
the "BEGIN" operation until the context managers are actually entered. This
is different from the legacy version which runs the "BEGIN" operation up
front.

Fixes: #7272
Change-Id: I9667ac0861a9e007c4b3dfcf0fcf0829038a8711
(cherry picked from commit c4abf5a)
@zzzeek zzzeek mentioned this issue Nov 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working engine engines, connections, transactions, isolation levels, execution options
Projects
None yet
Milestone
1.4.x
Development

No branches or pull requests
2 participants
@zzzeek @sqla-tester