diff --git a/src/informix_db/cursors.py b/src/informix_db/cursors.py index cd5a2bd..1221a21 100644 --- a/src/informix_db/cursors.py +++ b/src/informix_db/cursors.py @@ -381,6 +381,37 @@ class Cursor: if self._description is not None: self._row_index = -1 + def _release_after_failure(self, *, close_cursor: bool = False) -> None: + """Best-effort server-side cleanup after a statement fails. + + A statement that failed is still allocated. Skipping the release + bricks the connection: the next PREPARE collides with the leaked + one and every subsequent call returns a nonsense error whose + offset points back at the *failed* SQL. A duplicate-key violation + is about the most ordinary error an application can hit, so "one + constraint violation kills the connection" was easy to reach and + hard to attribute. + + CLOSE and RELEASE get **separate** suppressions. Putting both in + one ``with contextlib.suppress(Exception)`` block reads as "clean + up both", but a CLOSE that raises skips the RELEASE entirely — + and RELEASE is the one that matters. Failing to close a cursor + wastes a handle; failing to release the statement is what breaks + the next call. + + Everything here is swallowed rather than propagated. If the wire + is genuinely desynced then the cleanup fails too, and the caller + is far better served by the real SQL error than by a secondary + failure from the cleanup path. + """ + if close_cursor: + with contextlib.suppress(Exception): + self._conn._send_pdu(self._build_close_pdu()) + self._drain_to_eot() + with contextlib.suppress(Exception): + self._conn._send_pdu(self._build_release_pdu()) + self._drain_to_eot() + def _execute_select_with_params(self, params: tuple) -> None: """Parameterized SELECT: SQ_BIND → CURNAME+NFETCH → drain → CLOSE+RELEASE. @@ -399,12 +430,20 @@ class Cursor: try: pdu = self._build_bind_only_pdu(params) except Exception: - with contextlib.suppress(Exception): - self._conn._send_pdu(self._build_release_pdu()) - self._drain_to_eot() + self._release_after_failure() raise self._conn._send_pdu(pdu) - self._drain_to_eot() + try: + self._drain_to_eot() + except Exception: + # The server can reject the BIND itself — a value whose type + # doesn't match the described parameter, a bind against a + # statement the server has since invalidated. This was the + # last unguarded door of the six: the build was covered and + # the drain was not, so a server-side bind rejection left the + # statement allocated and the *next* execute() failed instead. + self._release_after_failure() + raise # Now open the cursor and fetch — the bound values are in scope # for the prepared statement. self._execute_select() @@ -427,7 +466,18 @@ class Cursor: self._conn._send_pdu( self._build_curname_scroll_open_pdu(cursor_name) ) - self._drain_to_eot() + try: + self._drain_to_eot() + except Exception: + # Opening a scrollable cursor fails like any other + # statement — a bad ORDER BY, a permission error, a table + # dropped between PREPARE and OPEN. This branch had no + # guard at all, and it is the worst place to lack one: + # the GC-time finalizer is armed on the line *after* the + # drain, so a failure here left the statement allocated + # with no fallback whatsoever to reclaim it. + self._release_after_failure(close_cursor=True) + raise self._server_cursor_open = True self._finalizer_state[0] = True # arm the GC-time fallback self._scroll_total_rows = None @@ -461,12 +511,9 @@ class Cursor: # that returns early), so this path needs its own cleanup — # otherwise a mid-fetch failure leaks and the next statement # collides with it. Same failure mode as the DML path; see - # _execute_dml for what that looks like from the caller's side. - with contextlib.suppress(Exception): - self._conn._send_pdu(self._build_close_pdu()) - self._drain_to_eot() - self._conn._send_pdu(self._build_release_pdu()) - self._drain_to_eot() + # _release_after_failure for what that looks like from the + # caller's side. + self._release_after_failure(close_cursor=True) raise self._conn._send_pdu(self._build_close_pdu()) @@ -853,9 +900,7 @@ class Cursor: try: pdu = self._build_bind_execute_pdu(params) except Exception: - with contextlib.suppress(Exception): - self._conn._send_pdu(self._build_release_pdu()) - self._drain_to_eot() + self._release_after_failure() raise self._conn._send_pdu(pdu) try: @@ -864,9 +909,7 @@ class Cursor: # The statement is still allocated server-side even though it # failed. See _execute_dml for why skipping this bricks the # connection. - with contextlib.suppress(Exception): - self._conn._send_pdu(self._build_release_pdu()) - self._drain_to_eot() + self._release_after_failure() raise self._conn._send_pdu(self._build_release_pdu()) self._drain_to_eot() @@ -906,9 +949,7 @@ class Cursor: # desynced the release will fail too, and the caller is far # better served by the real SQL error than by a secondary # failure from the cleanup path. - with contextlib.suppress(Exception): - self._conn._send_pdu(self._build_release_pdu()) - self._drain_to_eot() + self._release_after_failure() raise self._conn._send_pdu(self._build_release_pdu()) self._drain_to_eot() @@ -1022,9 +1063,7 @@ class Cursor: # already unusable in that case, but attempting the # release costs nothing and the original error is what # propagates either way. - with contextlib.suppress(Exception): - self._conn._send_pdu(self._build_release_pdu()) - self._drain_to_eot() + self._release_after_failure() raise # Drain N responses. The first error is captured but we diff --git a/tests/test_statement_release.py b/tests/test_statement_release.py new file mode 100644 index 0000000..8282b0a --- /dev/null +++ b/tests/test_statement_release.py @@ -0,0 +1,189 @@ +"""Every door out of a statement has to release it on the way through. + +A statement that fails is still allocated server-side. Skipping the +RELEASE bricks the connection: the next PREPARE collides with the leaked +one, and every subsequent call returns a nonsense error whose offset +points back at the *failed* SQL rather than the new statement. + +There are six exits from the execute paths, and the guard was added to +them one at a time, each after a user hit it: + +1. ``_execute_dml`` drain +2. ``_execute_dml_with_params`` build +3. ``_execute_dml_with_params`` drain +4. ``executemany`` pipeline build/send +5. ``_execute_select_with_params`` build +6. ``_execute_select`` fetch loop + +Two were still open, and both are ordinary to reach: + +* **The parameterized-SELECT bind drain.** The build was guarded and the + drain was not. Passing a string where the column is an INT gets a + clean encode, so the rejection comes from the *server* (-1213, -415) + during the bind drain — past the guard. A wrong-typed parameter is + about as common as application mistakes get. + +* **The scrollable-cursor open.** No guard at all, and the worst place + to lack one: the GC-time finalizer is armed on the line *after* the + drain, so a failure there left the statement allocated with no + fallback of any kind. + +The guard is now one helper rather than six hand-rolled copies, which +also fixed a defect in the copies that had a cursor to close: CLOSE and +RELEASE shared a single ``contextlib.suppress`` block, so a CLOSE that +raised skipped the RELEASE — losing the half that actually matters. +""" + +from __future__ import annotations + +import pytest + +import informix_db +from informix_db.cursors import _RELEASE_PDU, Cursor +from tests.conftest import ConnParams + +# --------------------------------------------------------------------------- +# The helper — no server needed +# --------------------------------------------------------------------------- + + +class _FakeConn: + def __init__(self) -> None: + self.sent: list[bytes] = [] + + def _send_pdu(self, pdu: bytes) -> None: + self.sent.append(pdu) + + +class _FakeCursor: + """Duck-types the four attributes ``_release_after_failure`` touches.""" + + def __init__(self, *, close_raises: bool = False) -> None: + self._conn = _FakeConn() + self._close_raises = close_raises + + def _build_close_pdu(self) -> bytes: + return b"CLOSE" + + def _build_release_pdu(self) -> bytes: + return _RELEASE_PDU + + def _drain_to_eot(self) -> None: + if self._close_raises and self._conn.sent[-1] == b"CLOSE": + raise OSError("wire went away mid-close") + + +def test_release_is_sent_even_when_close_fails() -> None: + """The regression in the hand-rolled copies. CLOSE and RELEASE shared + one suppress block, so a failing CLOSE swallowed the RELEASE — and a + lost cursor handle is a nuisance while a lost statement breaks the + next call.""" + cur = _FakeCursor(close_raises=True) + Cursor._release_after_failure(cur, close_cursor=True) + assert _RELEASE_PDU in cur._conn.sent, ( + "a CLOSE that raises must not prevent the RELEASE" + ) + + +def test_close_is_skipped_when_no_cursor_was_opened() -> None: + cur = _FakeCursor() + Cursor._release_after_failure(cur) + assert cur._conn.sent == [_RELEASE_PDU] + + +def test_cleanup_never_propagates() -> None: + """Cleanup runs inside an ``except``. If it raises, it replaces the + real SQL error with a secondary failure from the cleanup path, which + is strictly worse for the caller.""" + + class _Hostile(_FakeCursor): + def _drain_to_eot(self) -> None: + raise OSError("connection reset") + + Cursor._release_after_failure(_Hostile(), close_cursor=True) + + +# --------------------------------------------------------------------------- +# Against a real server +# --------------------------------------------------------------------------- + + +def _connect(conn_params: ConnParams) -> informix_db.Connection: + return informix_db.connect( + host=conn_params.host, + port=conn_params.port, + user=conn_params.user, + password=conn_params.password, + database=conn_params.database, + server=conn_params.server, + connect_timeout=10.0, + # Bounded: a leaked statement used to manifest as a hang. + read_timeout=25.0, + autocommit=True, + ) + + +@pytest.mark.integration +@pytest.mark.parametrize( + ("sql", "params"), + [ + pytest.param( + "SELECT tabid FROM systables WHERE tabid = ?", + ("not-an-int",), + id="string-for-int", + ), + pytest.param( + "SELECT ?::INT FROM systables WHERE tabid = 1", + (2**40,), + id="out-of-range-int", + ), + ], +) +def test_server_rejected_bind_releases_statement( + conn_params: ConnParams, sql: str, params: tuple +) -> None: + """A parameterized SELECT whose bind the *server* rejects. The value + encodes cleanly, so this lands past the build guard and in the drain + that had none. Repeated because a leak only shows on the call after + it — the first failure looks fine on its own.""" + with _connect(conn_params) as conn: + cur = conn.cursor() + for i in range(4): + with pytest.raises(informix_db.Error): + cur.execute(sql, params) + cur.execute("SELECT FIRST 1 tabid FROM systables") + assert cur.fetchone() is not None, f"broken after {i + 1} binds" + + +@pytest.mark.integration +def test_failed_scroll_open_releases_statement(conn_params: ConnParams) -> None: + """FOR UPDATE prepares cleanly and fails at OPEN with -526, which is + exactly the branch that had no guard.""" + with _connect(conn_params) as conn: + scroll = conn.cursor(scrollable=True) + for i in range(4): + with pytest.raises(informix_db.Error): + scroll.execute("SELECT tabid FROM systables FOR UPDATE") + assert not scroll._server_cursor_open, ( + "a failed open must not leave the cursor marked live" + ) + other = conn.cursor() + other.execute("SELECT FIRST 1 tabid FROM systables") + assert other.fetchone() is not None, f"broken after {i + 1} opens" + other.close() + scroll.close() + + +@pytest.mark.integration +def test_scrollable_cursor_still_works_after_a_failed_open( + conn_params: ConnParams, +) -> None: + """The same cursor object must be reusable — a failed open is an + ordinary error, not a terminal state for the cursor.""" + with _connect(conn_params) as conn: + scroll = conn.cursor(scrollable=True) + with pytest.raises(informix_db.Error): + scroll.execute("SELECT tabid FROM systables FOR UPDATE") + scroll.execute("SELECT tabid FROM systables ORDER BY tabid") + assert scroll.fetch_first() is not None + scroll.close()