Skip to content

Commit cfdb185

Browse files
Make AsyncCursor.close sync and harden execute against close-race
Three coupled defects on AsyncCursor close + execute, resolved together per the architect's hybrid plan: * AsyncCursor.close was ``async def`` despite a body with zero await statements (pure attribute writes + weakref-proxy swap). A forgotten ``await`` produced a discarded coroutine, the cursor stayed open, and the only signal was a GC-time RuntimeWarning("coroutine was never awaited") pointing at asyncio internals. Convert to plain ``def`` matching stdlib sqlite3.Cursor.close and the project's executescript / interrupt / backup / tpc_* family. * close did not acquire _op_lock and was racy with in-flight execute on a sibling task. Hybrid resolution: keep close sync but flip ``_closed = True`` FIRST (GIL-atomic write) so a sibling-task executor parked on the wire await observes the flag-flip on resume. * _execute_unlocked populated _description / _rows / _rowcount after the wire await without re-checking _closed. Add a post-await ``if self._closed: return`` short-circuit (both query and non- query branches) so a concurrent close does not get its state scrubbing overwritten by the late wire response. Mirrors the same discipline already applied in _ExecuteManyAccumulator.apply. Updated every ``await cur.close()`` call site in the dbapi/aio package and in the test fakes that mocked the old async-def contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent d8238da commit cfdb185

26 files changed

Lines changed: 195 additions & 69 deletions

src/dqlitedbapi/aio/connection.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1683,7 +1683,7 @@ async def execute(
16831683
await cur.execute(operation, parameters)
16841684
except BaseException:
16851685
with contextlib.suppress(Exception):
1686-
await cur.close()
1686+
cur.close()
16871687
raise
16881688
return cur
16891689

@@ -1729,13 +1729,13 @@ async def executemany(
17291729
_validate_executemany_seq_shape(seq_of_parameters)
17301730
except ProgrammingError:
17311731
with contextlib.suppress(Exception):
1732-
await cur.close()
1732+
cur.close()
17331733
raise
17341734
try:
17351735
await cur.executemany(operation, seq_of_parameters)
17361736
except BaseException:
17371737
with contextlib.suppress(Exception):
1738-
await cur.close()
1738+
cur.close()
17391739
raise
17401740
return cur
17411741

src/dqlitedbapi/aio/cursor.py

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,20 @@ async def _execute_unlocked(
306306
columns, column_types, row_types, rows = await _call_client(
307307
conn.query_raw_typed(operation, params)
308308
)
309+
# Post-await close-race guard: a sibling task may have
310+
# called ``self.close()`` while we were parked on the
311+
# wire. ``close()`` is synchronous and sets ``_closed =
312+
# True`` GIL-atomically, plus clears ``_rows`` /
313+
# ``_description``; without this re-check, the result-
314+
# population below would re-populate state onto the now-
315+
# closed cursor. Drop the result silently (close is a no-
316+
# error termination — raising into the awaiter's frame
317+
# would surprise a caller that just used a try/except for
318+
# CancelledError). Mirrors the same discipline already
319+
# applied in ``_ExecuteManyAccumulator.apply``'s post-
320+
# await re-check arm.
321+
if self._closed:
322+
return
309323
if not columns:
310324
# PRAGMA write-form dispatches through the row-
311325
# returning branch but produces no columns; match
@@ -358,6 +372,9 @@ async def _execute_unlocked(
358372
self._rowcount = len(rows)
359373
else:
360374
last_id, affected = await _call_client(conn.execute(operation, params))
375+
# Same post-await close-race guard as the query branch.
376+
if self._closed:
377+
return
361378
# stdlib-parity: lastrowid only updates on INSERT / REPLACE.
362379
# See ``_is_insert_or_replace`` in the sync cursor for
363380
# rationale — sync and async share the same contract.
@@ -873,10 +890,22 @@ def drain_rows(self) -> list[tuple[Any, ...]]:
873890
self._row_index = 0
874891
return rows
875892

876-
async def close(self) -> None:
893+
def close(self) -> None:
877894
"""Close the cursor.
878895
879-
Idempotent: a second call is a no-op.
896+
Idempotent and **synchronous by design**. The body has zero
897+
await statements (every operation is a GIL-atomic attribute
898+
write plus the weakref-proxy swap); making it ``async def``
899+
would invite a forgot-``await`` footgun where ``cur.close()``
900+
silently produced a discarded coroutine and the cursor was
901+
left undrained, with only a GC-time
902+
``RuntimeWarning("coroutine was never awaited")`` pointing at
903+
asyncio internals rather than at dqlite. Mirrors stdlib
904+
``sqlite3.Cursor.close`` and the project's
905+
``executescript`` / ``interrupt`` / ``backup`` / ``tpc_*``
906+
family (sync ``def`` stubs that surface forgot-call as
907+
immediate ``NotSupportedError`` rather than a discarded
908+
coroutine).
880909
881910
Scrubs ``description`` / ``rowcount`` / ``lastrowid`` /
882911
``_rows`` / ``_row_index`` symmetrically with the sync sibling
@@ -890,11 +919,21 @@ async def close(self) -> None:
890919
retain ``arraysize`` across ``close()``; this driver matches
891920
that parity. ``arraysize`` is therefore the single PEP 249
892921
§6.1.2 attribute outside the scrub set above — by design.
922+
923+
Cross-task safety: ``_closed = True`` is set FIRST (GIL-atomic
924+
write) so a sibling task whose ``_execute_unlocked`` is parked
925+
on the wire await observes the flag-flip on resume. The
926+
executor's post-await ``if self._closed: return`` short-circuit
927+
prevents the wire response from re-populating ``_rows`` /
928+
``_description`` onto a closed cursor.
893929
"""
894930
# PEP 249 §6.1.2 messages-clear contract; see Cursor.close.
895931
del self.messages[:]
896932
if self._closed:
897933
return
934+
# Set the flag FIRST so a sibling-task ``_execute_unlocked``
935+
# that is parked on the wire await observes it on resume and
936+
# short-circuits before repopulating state.
898937
self._closed = True
899938
self._rows = []
900939
self._description = None
@@ -918,7 +957,7 @@ async def close(self) -> None:
918957
): # pragma: no cover - AsyncConnection always supports weakref
919958
self._connection = weakref.proxy(self._connection)
920959

921-
def setinputsizes(self, sizes: Sequence[Any]) -> None:
960+
def setinputsizes(self, sizes: Sequence[Any] | None) -> None:
922961
"""Set input sizes (no-op for dqlite).
923962
924963
PEP 249 §6.1.1 names ``setinputsizes`` among the methods that
@@ -1208,4 +1247,5 @@ async def __aexit__(
12081247
exc_val: BaseException | None,
12091248
exc_tb: TracebackType | None,
12101249
) -> None:
1211-
await self.close()
1250+
# ``close`` is now sync (see docstring) — no await needed.
1251+
self.close()

src/dqlitedbapi/cursor.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2057,7 +2057,7 @@ def close(self) -> None:
20572057
): # pragma: no cover - Connection always supports weakref
20582058
self._connection = weakref.proxy(self._connection)
20592059

2060-
def setinputsizes(self, sizes: Sequence[Any]) -> None:
2060+
def setinputsizes(self, sizes: Sequence[Any] | None) -> None:
20612061
"""Set input sizes (no-op for dqlite).
20622062
20632063
PEP 249 §6.1.1 names ``setinputsizes`` among the methods that

tests/aio/test_async_iter_protocol_invariants.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ async def test_aiter_on_closed_cursor_does_not_raise() -> None:
3232
to ``fetchone`` and raises ``InterfaceError("Cursor is closed")``)."""
3333
aconn = AsyncConnection("127.0.0.1:9999", database="x")
3434
cur = aconn.cursor()
35-
await cur.close()
35+
cur.close()
3636

3737
# Must NOT raise. Pre-fix this raised InterfaceError because
3838
# ``_check_loop_binding`` ran a closed-state check before

tests/integration/test_description_after_ddl_pragma.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,6 @@ async def test_description_is_none_after_ddl(self) -> None:
102102

103103
await cur.execute("DROP TABLE t_ddl_pragma_async")
104104
assert cur.description is None
105-
await cur.close()
105+
cur.close()
106106
finally:
107107
await conn.close()

tests/integration/test_lastrowid_autoincrement.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,6 @@ async def test_lastrowid_after_insert(self) -> None:
8686
assert cur.lastrowid == first + 1
8787

8888
await cur.execute("DROP TABLE t_rowid_async")
89-
await cur.close()
89+
cur.close()
9090
finally:
9191
await aconn.close()

tests/integration/test_lastrowid_survives_cross_cursor_rollback.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ async def test_two_cursor_rollback_keeps_each_lastrowid(
9797
await setup.execute("DROP TABLE IF EXISTS t_xc_rollback_async")
9898
await setup.execute("CREATE TABLE t_xc_rollback_async (id INTEGER PRIMARY KEY, v TEXT)")
9999
finally:
100-
await setup.close()
100+
setup.close()
101101

102102
cur_a = aconn.cursor()
103103
cur_b = aconn.cursor()
@@ -120,10 +120,10 @@ async def test_two_cursor_rollback_keeps_each_lastrowid(
120120
row = await cur_a.fetchone()
121121
assert row is not None and row[0] == 0
122122
finally:
123-
await cur_a.close()
124-
await cur_b.close()
123+
cur_a.close()
124+
cur_b.close()
125125
cleanup = aconn.cursor()
126126
try:
127127
await cleanup.execute("DROP TABLE IF EXISTS t_xc_rollback_async")
128128
finally:
129-
await cleanup.close()
129+
cleanup.close()

tests/integration/test_no_transaction_error_wording.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ async def test_stray_commit_emits_known_error(
9999
msg = str(ei.value).lower()
100100
assert "no transaction is active" in msg, f"unexpected msg: {ei.value!s}"
101101
finally:
102-
await cur.close()
102+
cur.close()
103103

104104
async def test_stray_rollback_emits_known_error(
105105
self, aconn: dqlitedbapi.aio.AsyncConnection
@@ -112,7 +112,7 @@ async def test_stray_rollback_emits_known_error(
112112
msg = str(ei.value).lower()
113113
assert "no transaction is active" in msg, f"unexpected msg: {ei.value!s}"
114114
finally:
115-
await cur.close()
115+
cur.close()
116116

117117
async def test_commit_swallows_no_tx_via_connection_method(
118118
self, aconn: dqlitedbapi.aio.AsyncConnection

tests/test_async_cursor.py

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -37,14 +37,14 @@ def test_lastrowid_initially_none(self) -> None:
3737
async def test_close_marks_cursor_closed(self) -> None:
3838
conn = AsyncConnection("localhost:9001")
3939
cursor = AsyncCursor(conn)
40-
await cursor.close()
40+
cursor.close()
4141
assert cursor._closed
4242

4343
async def test_close_is_idempotent(self) -> None:
4444
conn = AsyncConnection("localhost:9001")
4545
cursor = AsyncCursor(conn)
46-
await cursor.close()
47-
await cursor.close() # must not raise
46+
cursor.close()
47+
cursor.close() # must not raise
4848
assert cursor._closed
4949

5050
def test_connection_property(self) -> None:
@@ -55,22 +55,22 @@ def test_connection_property(self) -> None:
5555
async def test_fetchone_on_closed_cursor_raises(self) -> None:
5656
conn = AsyncConnection("localhost:9001")
5757
cursor = AsyncCursor(conn)
58-
await cursor.close()
58+
cursor.close()
5959

6060
with pytest.raises(InterfaceError, match="Cursor is closed"):
6161
await cursor.fetchone()
6262

6363
async def test_fetchmany_on_closed_cursor_raises(self) -> None:
6464
conn = AsyncConnection("localhost:9001")
6565
cursor = AsyncCursor(conn)
66-
await cursor.close()
66+
cursor.close()
6767
with pytest.raises(InterfaceError, match="Cursor is closed"):
6868
await cursor.fetchmany(5)
6969

7070
async def test_fetchall_on_closed_cursor_raises(self) -> None:
7171
conn = AsyncConnection("localhost:9001")
7272
cursor = AsyncCursor(conn)
73-
await cursor.close()
73+
cursor.close()
7474
with pytest.raises(InterfaceError, match="Cursor is closed"):
7575
await cursor.fetchall()
7676

@@ -264,7 +264,7 @@ async def run_execute() -> None:
264264

265265
task = asyncio.create_task(run_execute())
266266
await ensure_entered.wait()
267-
await cursor.close()
267+
cursor.close()
268268
close_allowed.set()
269269
with pytest.raises(InterfaceError, match="Cursor is closed"):
270270
await task

tests/test_async_cursor_aenter_gc_parent_pep249.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ async def test_aenter_on_closed_cursor_with_gc_parent_raises_interface_error() -
2222
``ReferenceError`` out of ``__aenter__``."""
2323
conn = AsyncConnection("localhost:9001")
2424
cur = conn.cursor()
25-
await cur.close()
25+
cur.close()
2626
# Drop the connection ref and force GC so the proxy referent
2727
# disappears.
2828
del conn

0 commit comments

Comments
 (0)