From: Daniele Varrazzo Date: Sun, 28 Nov 2021 16:15:16 +0000 (+0100) Subject: Don't clobber exception if rollback fails on transaction exit X-Git-Tag: 3.0.5~1^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fef9ba0370ef3029bdcac6b3656f51be4bdc2a5e;p=thirdparty%2Fpsycopg.git 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 8da0b656f..cb117f0a3 100644 --- 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 8815c970c..ad6a5a900 100644 --- 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 5e0b8e9a5..783a8ad0d 100644 --- 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 2baa2b088..ebdd8cbd3 100644 --- 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 fed04d7ef..f60eb4641 100644 --- 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 e940a89ae..a32cc2f3b 100644 --- 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 ba54549c8..d6522e117 100644 --- 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 c1d767b6f..efcb1341c 100644 --- 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")