The last two doors out of a statement didn't release it
A statement that fails is still allocated server-side, and skipping the RELEASE bricks the connection: the next PREPARE collides with the leaked one and every later call returns a nonsense error whose offset points back at the failed SQL. That guard has now been added six times, once per exit, each after a user found the exit. Two were still open. The parameterized-SELECT bind drain. The build was guarded and the drain was not, and the drain is where the interesting failures land: passing a string where the column is an INT encodes cleanly, so the rejection comes from the server (-1213, -415) after the guard has been passed. A wrong-typed parameter is about as common as application mistakes get. The scrollable-cursor open, which had no guard at all. 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. SELECT ... FOR UPDATE reaches it: prepares clean, fails at OPEN with -526. All six now call one helper instead of hand-rolling the cleanup, which also fixes a defect in the two 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 matters, since a leaked cursor handle is a nuisance and a leaked statement breaks the next call. Each PDU gets its own suppression now. All four new integration tests fail against the previous commit.
This commit is contained in:
parent
66120cf6f3
commit
0d8bd57ba9
@ -381,6 +381,37 @@ class Cursor:
|
|||||||
if self._description is not None:
|
if self._description is not None:
|
||||||
self._row_index = -1
|
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:
|
def _execute_select_with_params(self, params: tuple) -> None:
|
||||||
"""Parameterized SELECT: SQ_BIND → CURNAME+NFETCH → drain → CLOSE+RELEASE.
|
"""Parameterized SELECT: SQ_BIND → CURNAME+NFETCH → drain → CLOSE+RELEASE.
|
||||||
|
|
||||||
@ -399,12 +430,20 @@ class Cursor:
|
|||||||
try:
|
try:
|
||||||
pdu = self._build_bind_only_pdu(params)
|
pdu = self._build_bind_only_pdu(params)
|
||||||
except Exception:
|
except Exception:
|
||||||
with contextlib.suppress(Exception):
|
self._release_after_failure()
|
||||||
self._conn._send_pdu(self._build_release_pdu())
|
|
||||||
self._drain_to_eot()
|
|
||||||
raise
|
raise
|
||||||
self._conn._send_pdu(pdu)
|
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
|
# Now open the cursor and fetch — the bound values are in scope
|
||||||
# for the prepared statement.
|
# for the prepared statement.
|
||||||
self._execute_select()
|
self._execute_select()
|
||||||
@ -427,7 +466,18 @@ class Cursor:
|
|||||||
self._conn._send_pdu(
|
self._conn._send_pdu(
|
||||||
self._build_curname_scroll_open_pdu(cursor_name)
|
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._server_cursor_open = True
|
||||||
self._finalizer_state[0] = True # arm the GC-time fallback
|
self._finalizer_state[0] = True # arm the GC-time fallback
|
||||||
self._scroll_total_rows = None
|
self._scroll_total_rows = None
|
||||||
@ -461,12 +511,9 @@ class Cursor:
|
|||||||
# that returns early), so this path needs its own cleanup —
|
# that returns early), so this path needs its own cleanup —
|
||||||
# otherwise a mid-fetch failure leaks and the next statement
|
# otherwise a mid-fetch failure leaks and the next statement
|
||||||
# collides with it. Same failure mode as the DML path; see
|
# collides with it. Same failure mode as the DML path; see
|
||||||
# _execute_dml for what that looks like from the caller's side.
|
# _release_after_failure for what that looks like from the
|
||||||
with contextlib.suppress(Exception):
|
# caller's side.
|
||||||
self._conn._send_pdu(self._build_close_pdu())
|
self._release_after_failure(close_cursor=True)
|
||||||
self._drain_to_eot()
|
|
||||||
self._conn._send_pdu(self._build_release_pdu())
|
|
||||||
self._drain_to_eot()
|
|
||||||
raise
|
raise
|
||||||
|
|
||||||
self._conn._send_pdu(self._build_close_pdu())
|
self._conn._send_pdu(self._build_close_pdu())
|
||||||
@ -853,9 +900,7 @@ class Cursor:
|
|||||||
try:
|
try:
|
||||||
pdu = self._build_bind_execute_pdu(params)
|
pdu = self._build_bind_execute_pdu(params)
|
||||||
except Exception:
|
except Exception:
|
||||||
with contextlib.suppress(Exception):
|
self._release_after_failure()
|
||||||
self._conn._send_pdu(self._build_release_pdu())
|
|
||||||
self._drain_to_eot()
|
|
||||||
raise
|
raise
|
||||||
self._conn._send_pdu(pdu)
|
self._conn._send_pdu(pdu)
|
||||||
try:
|
try:
|
||||||
@ -864,9 +909,7 @@ class Cursor:
|
|||||||
# The statement is still allocated server-side even though it
|
# The statement is still allocated server-side even though it
|
||||||
# failed. See _execute_dml for why skipping this bricks the
|
# failed. See _execute_dml for why skipping this bricks the
|
||||||
# connection.
|
# connection.
|
||||||
with contextlib.suppress(Exception):
|
self._release_after_failure()
|
||||||
self._conn._send_pdu(self._build_release_pdu())
|
|
||||||
self._drain_to_eot()
|
|
||||||
raise
|
raise
|
||||||
self._conn._send_pdu(self._build_release_pdu())
|
self._conn._send_pdu(self._build_release_pdu())
|
||||||
self._drain_to_eot()
|
self._drain_to_eot()
|
||||||
@ -906,9 +949,7 @@ class Cursor:
|
|||||||
# desynced the release will fail too, and the caller is far
|
# desynced the release will fail too, and the caller is far
|
||||||
# better served by the real SQL error than by a secondary
|
# better served by the real SQL error than by a secondary
|
||||||
# failure from the cleanup path.
|
# failure from the cleanup path.
|
||||||
with contextlib.suppress(Exception):
|
self._release_after_failure()
|
||||||
self._conn._send_pdu(self._build_release_pdu())
|
|
||||||
self._drain_to_eot()
|
|
||||||
raise
|
raise
|
||||||
self._conn._send_pdu(self._build_release_pdu())
|
self._conn._send_pdu(self._build_release_pdu())
|
||||||
self._drain_to_eot()
|
self._drain_to_eot()
|
||||||
@ -1022,9 +1063,7 @@ class Cursor:
|
|||||||
# already unusable in that case, but attempting the
|
# already unusable in that case, but attempting the
|
||||||
# release costs nothing and the original error is what
|
# release costs nothing and the original error is what
|
||||||
# propagates either way.
|
# propagates either way.
|
||||||
with contextlib.suppress(Exception):
|
self._release_after_failure()
|
||||||
self._conn._send_pdu(self._build_release_pdu())
|
|
||||||
self._drain_to_eot()
|
|
||||||
raise
|
raise
|
||||||
|
|
||||||
# Drain N responses. The first error is captured but we
|
# Drain N responses. The first error is captured but we
|
||||||
|
|||||||
189
tests/test_statement_release.py
Normal file
189
tests/test_statement_release.py
Normal file
@ -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()
|
||||||
Loading…
x
Reference in New Issue
Block a user