Don't clobber exception if rollback fails on transaction exit

Just raise a warning, consistently with what happens to the connection.

Close #165.

diff --git a/docs/news.rst b/docs/news.rst
index 8da0b656ffe00cdfafce26232a9680ab81b7126c..cb117f0a3efa7eb44cdabd3a3c7839eed873c3d0 100644 (file)
--- a/docs/news.rst
+++ b/docs/news.rst
@@ -15,6 +15,8 @@ Psycopg 3.0.5
 
 - Fix possible "Too many open files" OS error, reported on macOS but possible
   on other platforms too (:ticket:`#158`).
+- Don't clobber exceptions if a transaction block exit with error and rollback
+  fails (:ticket:`#165`).
 
 
 Psycopg 3.0.4

diff --git a/psycopg/psycopg/connection.py b/psycopg/psycopg/connection.py
index 8815c970c20223c3e9d34e7b342c1dcba7bd2622..ad6a5a9008fd8b37e79ab1dce71ddbb3dbb32423 100644 (file)
--- a/psycopg/psycopg/connection.py
+++ b/psycopg/psycopg/connection.py
@@ -593,7 +593,7 @@ class Connection(BaseConnection[Row]):
                 self.rollback()
             except Exception as exc2:
                 logger.warning(
-                    "error ignored rolling back transaction on %s: %s",
+                    "error ignored in rollback on %s: %s",
                     self,
                     exc2,
                 )

diff --git a/psycopg/psycopg/connection_async.py b/psycopg/psycopg/connection_async.py
index 5e0b8e9a57fb176ac4a1c5fa6672ba08d28eb7d9..783a8ad0dc540168fe9ac3fa2cb563f06da17643 100644 (file)
--- a/psycopg/psycopg/connection_async.py
+++ b/psycopg/psycopg/connection_async.py
@@ -138,7 +138,7 @@ class AsyncConnection(BaseConnection[Row]):
                 await self.rollback()
             except Exception as exc2:
                 logger.warning(
-                    "error ignored rolling back transaction on %s: %s",
+                    "error ignored in rollback on %s: %s",
                     self,
                     exc2,
                 )

diff --git a/psycopg/psycopg/transaction.py b/psycopg/psycopg/transaction.py
index 2baa2b088daaa324304ec9c43d2868429e3c8d03..ebdd8cbd38890378a780f74f14c04770531cdbd9 100644 (file)
--- a/psycopg/psycopg/transaction.py
+++ b/psycopg/psycopg/transaction.py
@@ -122,7 +122,17 @@ class BaseTransaction(Generic[ConnectionType]):
             yield from self._commit_gen()
             return False
         else:
-            return (yield from self._rollback_gen(exc_val))
+            # try to rollback, but if there are problems (connection in a bad
+            # state) just warn without clobbering the exception bubbling up.
+            try:
+                return (yield from self._rollback_gen(exc_val))
+            except Exception as exc2:
+                logger.warning(
+                    "error ignored in rollback of %s: %s",
+                    self,
+                    exc2,
+                )
+                return False
 
     def _commit_gen(self) -> PQGen[PGresult]:
         assert self._conn._savepoints[-1] == self._savepoint_name

diff --git a/tests/test_connection.py b/tests/test_connection.py
index fed04d7ef0d5a36b9849d215e73fa7925f5b08e8..f60eb4641ecbe5c14e5d7b70dde035a92635d357 100644 (file)
--- a/tests/test_connection.py
+++ b/tests/test_connection.py
@@ -167,7 +167,7 @@ def test_context_close(conn):
         conn.close()
 
 
-def test_context_rollback_no_clobber(conn, dsn, caplog):
+def test_context_inerror_rollback_no_clobber(conn, dsn, caplog):
     caplog.set_level(logging.WARNING, logger="psycopg")
 
     with pytest.raises(ZeroDivisionError):
@@ -182,7 +182,25 @@ def test_context_rollback_no_clobber(conn, dsn, caplog):
     assert len(caplog.records) == 1
     rec = caplog.records[0]
     assert rec.levelno == logging.WARNING
-    assert "rolling back" in rec.message
+    assert "in rollback" in rec.message
+
+
+def test_context_active_rollback_no_clobber(conn, dsn, caplog):
+    caplog.set_level(logging.WARNING, logger="psycopg")
+
+    with pytest.raises(ZeroDivisionError):
+        with psycopg.connect(dsn) as conn2:
+            with conn2.cursor() as cur:
+                with cur.copy(
+                    "copy (select generate_series(1, 10)) to stdout"
+                ) as copy:
+                    for row in copy.rows():
+                        1 / 0
+
+    assert len(caplog.records) == 1
+    rec = caplog.records[0]
+    assert rec.levelno == logging.WARNING
+    assert "in rollback" in rec.message
 
 
 @pytest.mark.slow

diff --git a/tests/test_connection_async.py b/tests/test_connection_async.py
index e940a89ae73f58e7a28603ddf8101a218274de34..a32cc2f3b28d8270a84cdb5f92f6e97e5faa071f 100644 (file)
--- a/tests/test_connection_async.py
+++ b/tests/test_connection_async.py
@@ -169,7 +169,7 @@ async def test_context_close(aconn):
         await aconn.close()
 
 
-async def test_context_rollback_no_clobber(conn, dsn, caplog):
+async def test_context_inerror_rollback_no_clobber(conn, dsn, caplog):
     with pytest.raises(ZeroDivisionError):
         async with await psycopg.AsyncConnection.connect(dsn) as conn2:
             await conn2.execute("select 1")
@@ -182,7 +182,25 @@ async def test_context_rollback_no_clobber(conn, dsn, caplog):
     assert len(caplog.records) == 1
     rec = caplog.records[0]
     assert rec.levelno == logging.WARNING
-    assert "rolling back" in rec.message
+    assert "in rollback" in rec.message
+
+
+async def test_context_active_rollback_no_clobber(conn, dsn, caplog):
+    caplog.set_level(logging.WARNING, logger="psycopg")
+
+    with pytest.raises(ZeroDivisionError):
+        async with await psycopg.AsyncConnection.connect(dsn) as conn2:
+            async with conn2.cursor() as cur:
+                async with cur.copy(
+                    "copy (select generate_series(1, 10)) to stdout"
+                ) as copy:
+                    async for row in copy.rows():
+                        1 / 0
+
+    assert len(caplog.records) == 1
+    rec = caplog.records[0]
+    assert rec.levelno == logging.WARNING
+    assert "in rollback" in rec.message
 
 
 @pytest.mark.slow

diff --git a/tests/test_transaction.py b/tests/test_transaction.py
index ba54549c8b1e5184e4f5bac5a44e9953d81da839..d6522e117dc4add866ce7a4e496e12c74185601f 100644 (file)
--- a/tests/test_transaction.py
+++ b/tests/test_transaction.py
@@ -1,3 +1,5 @@
+import logging
+
 import pytest
 
 from psycopg import Connection, ProgrammingError, Rollback
@@ -118,6 +120,45 @@ def test_rollback_on_exception_exit(conn):
     assert not inserted(conn)
 
 
+def test_context_inerror_rollback_no_clobber(conn, dsn, caplog):
+    caplog.set_level(logging.WARNING, logger="psycopg")
+
+    with pytest.raises(ZeroDivisionError):
+        with Connection.connect(dsn) as conn2:
+            with conn2.transaction():
+                conn2.execute("select 1")
+                conn.execute(
+                    "select pg_terminate_backend(%s::int)",
+                    [conn2.pgconn.backend_pid],
+                )
+                1 / 0
+
+    assert len(caplog.records) == 1
+    rec = caplog.records[0]
+    assert rec.levelno == logging.WARNING
+    assert "in rollback" in rec.message
+
+
+def test_context_active_rollback_no_clobber(conn, dsn, caplog):
+    caplog.set_level(logging.WARNING, logger="psycopg")
+
+    with pytest.raises(ZeroDivisionError):
+        conn2 = Connection.connect(dsn)
+        with conn2.transaction():
+            with conn2.cursor() as cur:
+                with cur.copy(
+                    "copy (select generate_series(1, 10)) to stdout"
+                ) as copy:
+                    for row in copy.rows():
+                        1 / 0
+
+    assert len(caplog.records) == 1
+    rec = caplog.records[0]
+    assert rec.levelno == logging.WARNING
+    assert "in rollback" in rec.message
+    conn2.close()
+
+
 def test_interaction_dbapi_transaction(conn):
     insert_row(conn, "foo")
 

diff --git a/tests/test_transaction_async.py b/tests/test_transaction_async.py
index c1d767b6f1bd0e98c93160b98c9c133620f27ca9..efcb1341ce6330e848c6e400b6737565cd1e9cdf 100644 (file)
--- a/tests/test_transaction_async.py
+++ b/tests/test_transaction_async.py
@@ -1,6 +1,8 @@
+import logging
+
 import pytest
 
-from psycopg import ProgrammingError, Rollback
+from psycopg import AsyncConnection, ProgrammingError, Rollback
 
 from .test_transaction import in_transaction, insert_row, inserted
 from .test_transaction import ExpectedException
@@ -71,6 +73,45 @@ async def test_rollback_on_exception_exit(aconn):
     assert not await inserted(aconn)
 
 
+async def test_context_inerror_rollback_no_clobber(aconn, dsn, caplog):
+    caplog.set_level(logging.WARNING, logger="psycopg")
+
+    with pytest.raises(ZeroDivisionError):
+        async with await AsyncConnection.connect(dsn) as conn2:
+            async with conn2.transaction():
+                await conn2.execute("select 1")
+                await aconn.execute(
+                    "select pg_terminate_backend(%s::int)",
+                    [conn2.pgconn.backend_pid],
+                )
+                1 / 0
+
+    assert len(caplog.records) == 1
+    rec = caplog.records[0]
+    assert rec.levelno == logging.WARNING
+    assert "in rollback" in rec.message
+
+
+async def test_context_active_rollback_no_clobber(aconn, dsn, caplog):
+    caplog.set_level(logging.WARNING, logger="psycopg")
+
+    with pytest.raises(ZeroDivisionError):
+        conn2 = await AsyncConnection.connect(dsn)
+        async with conn2.transaction():
+            async with conn2.cursor() as cur:
+                async with cur.copy(
+                    "copy (select generate_series(1, 10)) to stdout"
+                ) as copy:
+                    async for row in copy.rows():
+                        1 / 0
+
+    assert len(caplog.records) == 1
+    rec = caplog.records[0]
+    assert rec.levelno == logging.WARNING
+    assert "in rollback" in rec.message
+    await conn2.close()
+
+
 async def test_interaction_dbapi_transaction(aconn):
     await insert_row(aconn, "foo")