Skip to content

Commit dd587f5

Browse files
Remap loop-closed RuntimeError in _run_sync to OperationalError
``asyncio.run_coroutine_threadsafe`` raises bare ``RuntimeError("Event loop is closed")`` when the loop is closed between ``_ensure_loop()`` returning and the schedule call landing — the canonical race shape under SA's ``do_terminate`` from a finalizer thread, manual ``loop.close()`` from a test fixture, and SIGTERM-with-budget shutdown. Two consequences before this fix: - The bare RuntimeError escapes the PEP 249 ``Error`` hierarchy. SA's ``is_disconnect`` is gated on ``DatabaseError``; the connection is not invalidated correctly and the pool retains it. - The unscheduled coroutine emits ``RuntimeWarning("coroutine was never awaited")`` at GC — a warning whose traceback does not point at dqlite, sending operators chasing the wrong layer. The fix wraps the schedule call in ``try/except RuntimeError`` that calls ``coro.close()`` (suppresses the warning unconditionally on every RuntimeError arm) and remaps the closed-loop substring to ``OperationalError`` chained from the original RuntimeError. Other RuntimeErrors (notably "Non-thread-safe operation invoked on an event loop other than the current one" — a programmer-bug shape) propagate unchanged so they are not silently classified as database connection failures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 5287fa2 commit dd587f5

2 files changed

Lines changed: 177 additions & 1 deletion

File tree

src/dqlitedbapi/connection.py

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -987,7 +987,40 @@ def _run_sync[T](self, coro: Coroutine[Any, Any, T]) -> T:
987987
"use from another thread)"
988988
)
989989
loop = self._ensure_loop()
990-
future = asyncio.run_coroutine_threadsafe(coro, loop)
990+
try:
991+
future = asyncio.run_coroutine_threadsafe(coro, loop)
992+
except RuntimeError as e:
993+
# ``asyncio.run_coroutine_threadsafe`` raises bare
994+
# ``RuntimeError("Event loop is closed")`` when the
995+
# loop is closed between ``_ensure_loop()`` returning
996+
# and the schedule call landing — the canonical race
997+
# is a sibling thread (``do_terminate`` from a
998+
# finalizer thread, manual ``loop.close()``, SIGTERM-
999+
# with-budget shutdown). Without this catch the bare
1000+
# RuntimeError escapes the PEP 249 ``Error`` hierarchy
1001+
# (SA's ``is_disconnect`` is gated on ``DatabaseError``
1002+
# so it cannot classify the failure correctly), AND
1003+
# the unscheduled coroutine emits
1004+
# ``RuntimeWarning("coroutine was never awaited")`` at
1005+
# GC — a warning whose traceback does not point at
1006+
# dqlite, sending operators chasing the wrong layer.
1007+
# Close the coroutine and remap to ``OperationalError``
1008+
# (a ``DatabaseError`` subclass) with the original
1009+
# RuntimeError chained for diagnostics. Narrow the
1010+
# remap to the closed-loop substring so unrelated
1011+
# RuntimeErrors ("Non-thread-safe operation invoked on
1012+
# an event loop other than the current one" — a
1013+
# programmer-bug shape) propagate as themselves
1014+
# rather than being silently classified as a database
1015+
# connection failure. Close the coroutine on every
1016+
# arm so neither path leaks the unawaited-coroutine
1017+
# warning.
1018+
coro.close()
1019+
if "Event loop is closed" not in str(e):
1020+
raise
1021+
raise OperationalError(
1022+
f"event loop closed before coroutine could be scheduled: {e}"
1023+
) from e
9911024
try:
9921025
# Future.result() provides a happens-before memory barrier,
9931026
# ensuring all writes by the event loop thread are visible here.
Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,143 @@
1+
"""Pin: ``_run_sync`` does not let a bare ``RuntimeError`` from
2+
``asyncio.run_coroutine_threadsafe`` escape the PEP 249 ``Error``
3+
hierarchy, and does not leak the unawaited coroutine.
4+
5+
The race shape: ``_run_sync`` calls ``self._ensure_loop()`` to obtain
6+
a live loop reference, then ``asyncio.run_coroutine_threadsafe(coro,
7+
loop)``. A sibling thread closing the loop between those two calls
8+
(typical: ``do_terminate`` from a finalizer thread, SIGTERM-with-
9+
budget shutdown, manual ``loop.close()`` from a test fixture)
10+
triggers a bare ``RuntimeError`` from ``run_coroutine_threadsafe``.
11+
12+
Bare RuntimeError consequences:
13+
14+
1. Escapes the PEP 249 ``Error`` hierarchy. SA's ``is_disconnect`` is
15+
gated on ``DatabaseError``; a bare RuntimeError here breaks
16+
classification and the connection is not retried / invalidated
17+
correctly.
18+
2. The coroutine's lifecycle is broken — it was never scheduled, and
19+
nothing called ``coro.close()``. At GC, asyncio emits
20+
``RuntimeWarning("coroutine was never awaited")`` that does not
21+
point at dqlite, sending operators on a wild goose chase.
22+
23+
The fix wraps ``run_coroutine_threadsafe`` in ``try/except
24+
RuntimeError``, calls ``coro.close()`` to suppress the warning, and
25+
raises an ``OperationalError`` (a ``DatabaseError`` subclass that SA's
26+
``is_disconnect`` can classify) chained from the underlying
27+
RuntimeError so the diagnostic is preserved.
28+
"""
29+
30+
from __future__ import annotations
31+
32+
import asyncio
33+
import warnings
34+
from typing import Any
35+
from unittest.mock import patch
36+
37+
import pytest
38+
39+
import dqlitedbapi.exceptions as _dbapi_exc
40+
from dqlitedbapi.connection import Connection
41+
42+
43+
def _make_connection() -> Connection:
44+
"""Build a minimal sync Connection bypassing the constructor's
45+
cluster machinery; only the state needed for ``_run_sync`` is
46+
populated.
47+
"""
48+
conn = Connection.__new__(Connection)
49+
# Connection's __init__ would normally do this; we only need
50+
# _op_lock and _timeout for _run_sync's lock-acquire path.
51+
import threading
52+
53+
conn._op_lock = threading.Lock()
54+
conn._timeout = 5.0
55+
conn._closed_flag = [False]
56+
conn._async_conn = None # not needed for the schedule-time race
57+
conn._creator_pid = 0
58+
return conn
59+
60+
61+
async def _trivial_coro() -> int:
62+
return 42
63+
64+
65+
def test_run_sync_raises_pep249_error_when_loop_closed_at_schedule() -> None:
66+
"""A loop closed between ``_ensure_loop`` returning and
67+
``run_coroutine_threadsafe`` running must surface as a PEP 249
68+
``OperationalError`` (a ``DatabaseError`` subclass), NOT a bare
69+
``RuntimeError``.
70+
"""
71+
conn = _make_connection()
72+
closed_loop = asyncio.new_event_loop()
73+
closed_loop.close() # simulate the race outcome
74+
75+
with patch.object(conn, "_ensure_loop", return_value=closed_loop):
76+
with pytest.raises(_dbapi_exc.Error) as exc_info:
77+
conn._run_sync(_trivial_coro())
78+
# Specifically OperationalError so SA's is_disconnect can
79+
# classify it, with the underlying RuntimeError chained on
80+
# __cause__ for diagnostics.
81+
assert isinstance(exc_info.value, _dbapi_exc.OperationalError), (
82+
f"expected OperationalError, got {type(exc_info.value).__name__}"
83+
)
84+
assert isinstance(exc_info.value.__cause__, RuntimeError), (
85+
f"original RuntimeError must be chained; got {type(exc_info.value.__cause__).__name__}"
86+
)
87+
88+
89+
def test_run_sync_does_not_leak_unawaited_coroutine_warning() -> None:
90+
"""The fix must call ``coro.close()`` on the schedule-failure
91+
path so asyncio's ``RuntimeWarning("coroutine was never awaited")``
92+
does NOT fire at GC. Without ``coro.close()`` the warning surfaces
93+
in caller code with no dqlite frame in the traceback, sending
94+
operators chasing the wrong layer.
95+
"""
96+
conn = _make_connection()
97+
closed_loop = asyncio.new_event_loop()
98+
closed_loop.close()
99+
100+
coro = _trivial_coro()
101+
with patch.object(conn, "_ensure_loop", return_value=closed_loop):
102+
with warnings.catch_warnings(record=True) as captured:
103+
warnings.simplefilter("always")
104+
with pytest.raises(_dbapi_exc.OperationalError):
105+
conn._run_sync(coro)
106+
# Force coroutine GC to surface the warning if it would
107+
# have fired.
108+
del coro
109+
import gc
110+
111+
gc.collect()
112+
unawaited = [
113+
w
114+
for w in captured
115+
if issubclass(w.category, RuntimeWarning)
116+
and "coroutine was never awaited" in str(w.message)
117+
]
118+
assert not unawaited, (
119+
f"unawaited-coroutine warning leaked from _run_sync's "
120+
f"schedule-failure path: {[str(w.message) for w in unawaited]}"
121+
)
122+
123+
124+
def test_run_sync_propagates_runtimeerror_message_in_cause() -> None:
125+
"""The OperationalError raised from the schedule-failure path
126+
must carry the underlying RuntimeError as ``__cause__`` so
127+
operators reading the traceback see the actual loop-closed signal,
128+
not just the dqlite-level remap.
129+
"""
130+
conn = _make_connection()
131+
closed_loop = asyncio.new_event_loop()
132+
closed_loop.close()
133+
134+
with (
135+
patch.object(conn, "_ensure_loop", return_value=closed_loop),
136+
pytest.raises(_dbapi_exc.OperationalError) as exc_info,
137+
):
138+
conn._run_sync(_trivial_coro())
139+
cause: Any = exc_info.value.__cause__
140+
assert isinstance(cause, RuntimeError)
141+
# asyncio's wording is "Event loop is closed" — pin a non-empty
142+
# str(cause) so the diagnostic carries through.
143+
assert str(cause), "RuntimeError cause must carry a non-empty message"

0 commit comments

Comments
 (0)