From 3024788129c63748b8ebcced8f165bb5ca3f1bc4 Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Thu, 9 Dec 2021 00:28:44 +0100 Subject: [PATCH] Raise ProgrammingError on out-of-order exit from transactions Previously it would have failed an assert. Further back in time, the condition was checked and reported as ProgrammingError already. The check was dropped when `conn.transaction()` started to return an entered transaction, so the possibility of calling enter/exit manually was taken out of the public API. However, we didn't consider the possibility of concurrent threads operating on transaction independently. Also fix the Transaction representation, which wouldn't have reported `(terminated)` exiting on rollback, but only on commit. Close #176. --- docs/news.rst | 2 ++ psycopg/psycopg/transaction.py | 38 +++++++++++++++++++++++++++------ tests/test_transaction.py | 28 ++++++++++++++++++++---- tests/test_transaction_async.py | 28 ++++++++++++++++++++---- 4 files changed, 81 insertions(+), 15 deletions(-) diff --git a/docs/news.rst b/docs/news.rst index aa22ec6f5..c02bf5c9d 100644 --- a/docs/news.rst +++ b/docs/news.rst @@ -22,6 +22,8 @@ Psycopg 3.0.6 (unreleased) - Don't raise exceptions on `ServerCursor.close()` if the connection is closed (:ticket:`#173`). - Fail on `Connection.cursor()` if the connection is closed (:ticket:`#174`). +- Raise `ProgrammingError` if out-of-order exit from transaction contexts is + detected (:ticket:`#176`). - Add `!CHECK_STANDBY` value to `~pq.ConnStatus` enum. diff --git a/psycopg/psycopg/transaction.py b/psycopg/psycopg/transaction.py index 4d01e836f..63aa01e34 100644 --- a/psycopg/psycopg/transaction.py +++ b/psycopg/psycopg/transaction.py @@ -11,6 +11,7 @@ from typing import Generic, Optional, Type, Union, TYPE_CHECKING from . import pq from . import sql +from . import errors as e from .pq import TransactionStatus from .abc import ConnectionType, PQGen from .pq.abc import PGresult @@ -44,6 +45,10 @@ class Rollback(Exception): return f"{self.__class__.__qualname__}({self.transaction!r})" +class OutOfOrderTransactionNesting(e.ProgrammingError): + """Out-of-order transaction nesting detected""" + + class BaseTransaction(Generic[ConnectionType]): def __init__( self, @@ -126,18 +131,24 @@ class BaseTransaction(Generic[ConnectionType]): # state) just warn without clobbering the exception bubbling up. try: return (yield from self._rollback_gen(exc_val)) + except OutOfOrderTransactionNesting: + # Clobber an exception happened in the block with the exception + # caused by out-of-order transaction detected, so make the + # behaviour consistent with _commit_gen and to make sure the + # user fixes this condition, which is unrelated from + # operational error that might arise in the block. + raise except Exception as exc2: logger.warning( - "error ignored in rollback of %s: %s", - self, - exc2, + "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 - self._conn._savepoints.pop() + ex = self._pop_savepoint("commit") self._exited = True + if ex: + raise ex commands = [] if self._savepoint_name and not self._outer_transaction: @@ -159,8 +170,10 @@ class BaseTransaction(Generic[ConnectionType]): f"{self._conn}: Explicit rollback from: ", exc_info=True ) - assert self._conn._savepoints[-1] == self._savepoint_name - self._conn._savepoints.pop() + ex = self._pop_savepoint("roll back") + self._exited = True + if ex: + raise ex commands = [] if self._savepoint_name and not self._outer_transaction: @@ -187,6 +200,17 @@ class BaseTransaction(Generic[ConnectionType]): return False + def _pop_savepoint(self, action: str) -> Optional[Exception]: + sp = self._conn._savepoints.pop() + if sp == self._savepoint_name: + return None + + other = f"the savepoint {sp!r}" if sp else "the top-level transaction" + return OutOfOrderTransactionNesting( + f"transactions not correctly nested: {self} would {action}" + f" in the wrong order compared to {other}" + ) + class Transaction(BaseTransaction["Connection[Any]"]): """ diff --git a/tests/test_transaction.py b/tests/test_transaction.py index f569bfc61..07a05bcbd 100644 --- a/tests/test_transaction.py +++ b/tests/test_transaction.py @@ -650,21 +650,41 @@ def test_str(conn): assert "[IDLE]" in str(tx) assert "(terminated)" in str(tx) + with pytest.raises(ZeroDivisionError): + with conn.transaction() as tx: + 1 / 0 + + assert "(terminated)" in str(tx) -@pytest.mark.parametrize("fail", [False, True]) -def test_concurrency(conn, fail): + +@pytest.mark.parametrize("what", ["commit", "rollback", "error"]) +def test_concurrency(conn, what): conn.autocommit = True e = [Event() for i in range(3)] def worker(unlock, wait_on): - with pytest.raises(ProgrammingError): + with pytest.raises(ProgrammingError) as ex: with conn.transaction(): unlock.set() wait_on.wait() conn.execute("select 1") - if fail: + + if what == "error": 1 / 0 + elif what == "rollback": + raise Rollback() + else: + assert what == "commit" + + if what == "error": + assert "would roll back" in str(ex.value) + assert isinstance(ex.value.__context__, ZeroDivisionError) + elif what == "rollback": + assert "would roll back" in str(ex.value) + assert isinstance(ex.value.__context__, Rollback) + else: + assert "would commit" in str(ex.value) # Start a first transaction in a thread t1 = Thread(target=worker, kwargs={"unlock": e[0], "wait_on": e[1]}) diff --git a/tests/test_transaction_async.py b/tests/test_transaction_async.py index 6b108d57a..4bfce1595 100644 --- a/tests/test_transaction_async.py +++ b/tests/test_transaction_async.py @@ -618,21 +618,41 @@ async def test_str(aconn): assert "[IDLE]" in str(tx) assert "(terminated)" in str(tx) + with pytest.raises(ZeroDivisionError): + async with aconn.transaction() as tx: + 1 / 0 + + assert "(terminated)" in str(tx) -@pytest.mark.parametrize("fail", [False, True]) -async def test_concurrency(aconn, fail): + +@pytest.mark.parametrize("what", ["commit", "rollback", "error"]) +async def test_concurrency(aconn, what): await aconn.set_autocommit(True) e = [asyncio.Event() for i in range(3)] async def worker(unlock, wait_on): - with pytest.raises(ProgrammingError): + with pytest.raises(ProgrammingError) as ex: async with aconn.transaction(): unlock.set() await wait_on.wait() await aconn.execute("select 1") - if fail: + + if what == "error": 1 / 0 + elif what == "rollback": + raise Rollback() + else: + assert what == "commit" + + if what == "error": + assert "would roll back" in str(ex.value) + assert isinstance(ex.value.__context__, ZeroDivisionError) + elif what == "rollback": + assert "would roll back" in str(ex.value) + assert isinstance(ex.value.__context__, Rollback) + else: + assert "would commit" in str(ex.value) # Start a first transaction in a task t1 = create_task(worker(unlock=e[0], wait_on=e[1])) -- 2.47.3