Run handle_error for any exceptions raised in execute_context()

Observing a SQLite connection/cursor being hung on
test_resultset -> PositionalTextTest -> test_dupe_col_obj.
this uses connectionless execution and the result object
fails to be constructed.  When that happens, there is no path
for the cursor or connection to be closed / released.
Recent changes with the exception assertions in #4849 seem to
be causing a cycle to last a little longer than usual which
is exposing this issue for one particular test on SQLite.

As we want to get rid of weakref cleanup, evaluate
why we dont have handle_dbapi_exception for this whole
block, as after_cursor_execute can raise, result construction
can raise, autocommit can raise, close can raise, there
does not seem to be a reason these things should be outside
of the block that gets cleaned up.

Fixes: #5182
Change-Id: I640ac55e8c5f39d287f779fbb5dc0ab727218ca3
(cherry picked from commit 4cd33f18a21d9e33b37ef7163822c327453d1e62)

diff --git a/doc/build/changelog/unreleased_13/5182.rst b/doc/build/changelog/unreleased_13/5182.rst
new file mode 100644 (file)
index 0000000..0512385
--- /dev/null
+++ b/doc/build/changelog/unreleased_13/5182.rst
@@ -0,0 +1,11 @@
+.. change::
+    :tags: bug, engine
+    :tickets: 5182
+
+    Expanded the scope of cursor/connection cleanup when a statement is
+    executed to include when the result object fails to be constructed, or an
+    after_cursor_execute() event raises an error, or autocommit / autoclose
+    fails.  This allows the DBAPI cursor to be cleaned up on failure and for
+    connectionless execution allows the connection to be closed out and
+    returned to the connection pool, where previously it waiting until garbage
+    collection would trigger a pool return.

diff --git a/lib/sqlalchemy/engine/base.py b/lib/sqlalchemy/engine/base.py
index aa6c2ee2515f4675dd76dcfa1311bea9662706ef..d58eeb1ea0264370e1b642c9bfcd82c0a9bca89a 100644 (file)
--- a/lib/sqlalchemy/engine/base.py
+++ b/lib/sqlalchemy/engine/base.py
@@ -1247,45 +1247,47 @@ class Connection(Connectable):
                     self.dialect.do_execute(
                         cursor, statement, parameters, context
                     )
-        except BaseException as e:
-            self._handle_dbapi_exception(
-                e, statement, parameters, cursor, context
-            )
 
-        if self._has_events or self.engine._has_events:
-            self.dispatch.after_cursor_execute(
-                self,
-                cursor,
-                statement,
-                parameters,
-                context,
-                context.executemany,
-            )
+            if self._has_events or self.engine._has_events:
+                self.dispatch.after_cursor_execute(
+                    self,
+                    cursor,
+                    statement,
+                    parameters,
+                    context,
+                    context.executemany,
+                )
 
-        if context.compiled:
-            context.post_exec()
+            if context.compiled:
+                context.post_exec()
 
-        if context.is_crud or context.is_text:
-            result = context._setup_crud_result_proxy()
-        else:
-            result = context.get_result_proxy()
-            if result._metadata is None:
-                result._soft_close()
+            if context.is_crud or context.is_text:
+                result = context._setup_crud_result_proxy()
+            else:
+                result = context.get_result_proxy()
+                if result._metadata is None:
+                    result._soft_close()
 
-        if context.should_autocommit and self._root.__transaction is None:
-            self._root._commit_impl(autocommit=True)
+            if context.should_autocommit and self._root.__transaction is None:
+                self._root._commit_impl(autocommit=True)
+
+            # for "connectionless" execution, we have to close this
+            # Connection after the statement is complete.
+            if self.should_close_with_result:
+                # ResultProxy already exhausted rows / has no rows.
+                # close us now
+                if result._soft_closed:
+                    self.close()
+                else:
+                    # ResultProxy will close this Connection when no more
+                    # rows to fetch.
+                    result._autoclose_connection = True
+
+        except BaseException as e:
+            self._handle_dbapi_exception(
+                e, statement, parameters, cursor, context
+            )
 
-        # for "connectionless" execution, we have to close this
-        # Connection after the statement is complete.
-        if self.should_close_with_result:
-            # ResultProxy already exhausted rows / has no rows.
-            # close us now
-            if result._soft_closed:
-                self.close()
-            else:
-                # ResultProxy will close this Connection when no more
-                # rows to fetch.
-                result._autoclose_connection = True
         return result
 
     def _cursor_execute(self, cursor, statement, parameters, context=None):

diff --git a/test/engine/test_execute.py b/test/engine/test_execute.py
index 93affccbd0670917c7509afbbe1d1c9400f169f0..6ff95ca4451a5ff9ccde065a89325ea6897d0e36 100644 (file)
--- a/test/engine/test_execute.py
+++ b/test/engine/test_execute.py
@@ -17,6 +17,7 @@ from sqlalchemy import select
 from sqlalchemy import Sequence
 from sqlalchemy import String
 from sqlalchemy import testing
+from sqlalchemy import text
 from sqlalchemy import TypeDecorator
 from sqlalchemy import util
 from sqlalchemy import VARCHAR
@@ -2353,6 +2354,83 @@ class HandleErrorTest(fixtures.TestBase):
             ):
                 assert_raises(MySpecialException, conn.get_isolation_level)
 
+    @testing.only_on("sqlite")
+    def test_cursor_close_resultset_failed_connectionless(self):
+        engine = engines.testing_engine()
+
+        the_conn = []
+        the_cursor = []
+
+        @event.listens_for(engine, "after_cursor_execute")
+        def go(
+            connection, cursor, statement, parameters, context, executemany
+        ):
+            the_cursor.append(cursor)
+            the_conn.append(connection)
+
+        c1, c2 = column("a"), column("b")
+
+        with mock.patch(
+            "sqlalchemy.engine.result.ResultProxy",
+            Mock(side_effect=tsa.exc.InvalidRequestError("duplicate col")),
+        ):
+            # result should fail
+            assert_raises(
+                tsa.exc.InvalidRequestError,
+                engine.execute,
+                text("select 1 as a, 2 as q, 3 as z").columns(c1, c2, c2),
+            )
+
+        # cursor is closed
+        assert_raises_message(
+            engine.dialect.dbapi.ProgrammingError,
+            "Cannot operate on a closed cursor",
+            the_cursor[0].execute,
+            "select 1",
+        )
+
+        # connection is closed
+        assert the_conn[0].closed
+
+    @testing.only_on("sqlite")
+    def test_cursor_close_resultset_failed_explicit(self):
+        engine = engines.testing_engine()
+
+        the_cursor = []
+
+        @event.listens_for(engine, "after_cursor_execute")
+        def go(
+            connection, cursor, statement, parameters, context, executemany
+        ):
+            the_cursor.append(cursor)
+
+        c1, c2 = column("a"), column("b")
+
+        conn = engine.connect()
+
+        with mock.patch(
+            "sqlalchemy.engine.result.ResultProxy",
+            Mock(side_effect=tsa.exc.InvalidRequestError("duplicate col")),
+        ):
+            assert_raises(
+                tsa.exc.InvalidRequestError,
+                conn.execute,
+                text("select 1 as a, 2 as q, 3 as z").columns(c1, c2, c2),
+            )
+
+        # cursor is closed
+        assert_raises_message(
+            engine.dialect.dbapi.ProgrammingError,
+            "Cannot operate on a closed cursor",
+            the_cursor[0].execute,
+            "select 1",
+        )
+
+        # connection not closed
+        assert not conn.closed
+
+        conn.close()
+
 
 class HandleInvalidatedOnConnectTest(fixtures.TestBase):
     __requires__ = ("sqlite",)