Skip to content

Commit 4b299dc

Browse files
Unblock a concurrent _run_sync caller on force_close_transport
force_close_transport() stopped the loop without resolving an outstanding run_coroutine_threadsafe future, so a concurrent _run_sync caller (under check_same_thread=False) waited out the full sync_timeout (4×timeout, ~40s by default) before a clean OperationalError. Set a dedicated _force_close_requested flag before stopping the loop — distinct from _closed_flag, which close() sets and then runs its own _run_sync(_close_async()) that must not bail — and wait on the future in short slices so the caller observes the flag and raises InterfaceError promptly. When the flag is unset the slice loop is behaviourally identical: it returns on completion, propagates a coroutine error, and re-raises the genuine TimeoutError at the deadline into the existing timeout arm. The _run_sync timeout stubs keyed on a call counter that assumed a single result() call; rework them to key on the actual control flow (done() checked for the recovery arm, cancel() called for the bounded wait) so they hold under the sliced wait. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent dfb4594 commit 4b299dc

3 files changed

Lines changed: 141 additions & 29 deletions

File tree

src/dqlitedbapi/connection.py

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import math
88
import os
99
import threading
10+
import time
1011
import warnings
1112
import weakref
1213
from collections.abc import Coroutine, Iterable, Iterator, Sequence
@@ -130,6 +131,11 @@ def _join_budget_for_current_thread(
130131
# N=4 covers the worst case: handshake + open_database + send +
131132
# read+drain. Steady-state bottoms out at N=2.
132133
_SYNC_PHASES_MULTIPLIER: Final[int] = 4
134+
# Slice length for _run_sync's result wait: short enough that a concurrent
135+
# force_close_transport unblocks the caller promptly (it sets
136+
# _force_close_requested before stopping the loop), long enough that a normal
137+
# in-flight op adds no measurable wakeup overhead.
138+
_FORCE_CLOSE_POLL_INTERVAL: Final[float] = 0.05
133139

134140
# Fallback join budget for ``_cleanup_loop_thread`` when the captured
135141
# ``close_timeout`` is missing or invalid.
@@ -1014,6 +1020,12 @@ def __init__(
10141020
# Mutable flag the finalizer reads; a list avoids the finalizer
10151021
# closing over ``self`` and preventing GC.
10161022
self._closed_flag: list[bool] = [False]
1023+
# Set by force_close_transport before it abruptly stops the loop, so a
1024+
# concurrent _run_sync (check_same_thread=False) bails promptly instead of
1025+
# riding out sync_timeout on a future the stopped loop will never resolve.
1026+
# Deliberately NOT _closed_flag: close() sets that and then runs its own
1027+
# _run_sync(self._close_async()), which must not bail.
1028+
self._force_close_requested: bool = False
10171029
# Box for late-publishing the inner handle into the finalizer's
10181030
# captured args (mutated by _get_async_connection to a
10191031
# weakref.ref). Not cleared on explicit close (finalizer is
@@ -1208,8 +1220,36 @@ def _run_sync[T](self, coro: Coroutine[Any, Any, T]) -> T:
12081220
f"event loop closed before coroutine could be scheduled: {e}"
12091221
) from e
12101222
# Future.result() is a happens-before barrier; loop-thread
1211-
# writes are visible here.
1212-
return future.result(timeout=sync_timeout)
1223+
# writes are visible here. Wait in slices (rather than one
1224+
# result(timeout=sync_timeout)) so a concurrent
1225+
# force_close_transport — which sets _force_close_requested
1226+
# and then stops the loop, orphaning this future — unblocks us
1227+
# promptly instead of riding out the full sync_timeout. When the
1228+
# flag is not set this is behaviourally identical: it re-raises
1229+
# the genuine TimeoutError at the deadline into the arm below.
1230+
deadline = time.monotonic() + sync_timeout
1231+
while True:
1232+
remaining = deadline - time.monotonic()
1233+
try:
1234+
return future.result(
1235+
timeout=max(0.0, min(_FORCE_CLOSE_POLL_INTERVAL, remaining))
1236+
)
1237+
except TimeoutError:
1238+
if self._force_close_requested:
1239+
# Honour a result that already landed (avoid a
1240+
# spurious failure / non-idempotent retry), else bail
1241+
# with the closed-connection contract rather than the
1242+
# timeout/invalidate path below.
1243+
if future.done() and not future.cancelled():
1244+
return future.result(timeout=0)
1245+
future.cancel()
1246+
raise InterfaceError(
1247+
f"Connection force-closed during operation (id={id(self)})"
1248+
) from None
1249+
if remaining <= _FORCE_CLOSE_POLL_INTERVAL:
1250+
# Genuine sync_timeout: re-raise to the arm below
1251+
# (recovered-result check + invalidate).
1252+
raise
12131253
except TimeoutError as e:
12141254
# Only future.result can raise this, so future is bound;
12151255
# assert is for mypy.
@@ -1570,6 +1610,9 @@ def force_close_transport(self) -> None:
15701610
if self._closed:
15711611
return
15721612
self._closed = True
1613+
# Signal a concurrent in-flight _run_sync to bail BEFORE we stop the loop
1614+
# (its future would otherwise never resolve). Set before any teardown.
1615+
self._force_close_requested = True
15731616
self._closed_flag[0] = True
15741617
# Fork-after-init: same shape as close()'s pid guard.
15751618
if get_current_pid() != self._creator_pid:
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
"""force_close_transport() must unblock a concurrent in-flight _run_sync caller promptly,
2+
not leave it riding out the full sync_timeout (4×timeout) on a future the stopped loop will
3+
never resolve. (check_same_thread=False; force_close is the documented last-resort path.)
4+
"""
5+
6+
from __future__ import annotations
7+
8+
import asyncio
9+
import contextlib
10+
import threading
11+
import time
12+
13+
import pytest
14+
15+
import dqlitedbapi
16+
from dqlitedbapi import InterfaceError, OperationalError
17+
18+
19+
@pytest.mark.integration
20+
def test_force_close_unblocks_concurrent_run_sync(cluster_address: str) -> None:
21+
# sync_timeout = 4 * 5.0 = 20s; pre-fix the concurrent caller wedged the full 20s.
22+
conn = dqlitedbapi.connect(cluster_address, timeout=5.0, check_same_thread=False)
23+
try:
24+
conn.cursor().execute("SELECT 1").fetchone() # warm up the loop + wire
25+
26+
result: dict[str, object] = {}
27+
ready = threading.Event()
28+
29+
def slow_op() -> None:
30+
ready.set()
31+
t0 = time.monotonic()
32+
try:
33+
conn._run_sync(asyncio.sleep(30)) # stand-in for a slow in-flight wire op
34+
except BaseException as e: # noqa: BLE001
35+
result["exc"] = e
36+
result["elapsed"] = time.monotonic() - t0
37+
38+
t = threading.Thread(target=slow_op)
39+
t.start()
40+
ready.wait(timeout=2.0)
41+
time.sleep(0.3) # let _run_sync acquire the op_lock and enter the result wait
42+
43+
conn.force_close_transport()
44+
45+
t.join(timeout=10.0)
46+
assert not t.is_alive(), "concurrent _run_sync caller did not unblock"
47+
assert "exc" in result, "the in-flight op did not raise on force-close"
48+
elapsed = result["elapsed"]
49+
assert isinstance(elapsed, float)
50+
# The whole point: unblocked promptly, NOT after the 20s sync_timeout.
51+
assert elapsed < 3.0, f"caller wedged {elapsed:.1f}s (sync_timeout was 20s)"
52+
assert isinstance(result["exc"], (InterfaceError, OperationalError))
53+
finally:
54+
with contextlib.suppress(Exception):
55+
conn.close()

tests/test_run_sync_timeout.py

Lines changed: 41 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,13 @@ def test_run_sync_logs_unexpected_error_during_cancel_wait(
3333

3434
class _StubFuture:
3535
def __init__(self) -> None:
36-
self._calls = 0
36+
self._cancel_called = False
3737

3838
def cancel(self) -> bool:
39+
# The except-TimeoutError arm calls cancel() before the bounded wait;
40+
# flip so only that bounded wait raises the surprise error, not the
41+
# slice-poll's repeated result() calls.
42+
self._cancel_called = True
3943
return True
4044

4145
def done(self) -> bool:
@@ -46,10 +50,9 @@ def cancelled(self) -> bool:
4650
return False
4751

4852
def result(self, timeout: float | None = None) -> None:
49-
self._calls += 1
50-
if self._calls == 1:
51-
raise cf.TimeoutError()
52-
raise RuntimeError("surprise bug during cancel")
53+
if self._cancel_called:
54+
raise RuntimeError("surprise bug during cancel")
55+
raise cf.TimeoutError()
5356

5457
stub = _StubFuture()
5558

@@ -128,22 +131,25 @@ class _LateSuccessFuture:
128131
"""result() raises TimeoutError first, then returns the late success."""
129132

130133
def __init__(self) -> None:
131-
self._calls = 0
134+
self._done_checked = False
132135

133136
def cancel(self) -> bool:
134137
return False # cancel lost the race
135138

136139
def done(self) -> bool:
140+
# Only the except-TimeoutError recovery arm calls done(); flip so
141+
# result() yields the late success only there, not during the
142+
# slice-poll's repeated result() calls.
143+
self._done_checked = True
137144
return True
138145

139146
def cancelled(self) -> bool:
140147
return False
141148

142149
def result(self, timeout: float | None = None) -> int:
143-
self._calls += 1
144-
if self._calls == 1:
145-
raise cf.TimeoutError()
146-
return 1234
150+
if self._done_checked:
151+
return 1234
152+
raise cf.TimeoutError()
147153

148154
stub = _LateSuccessFuture()
149155

@@ -184,24 +190,27 @@ class _LateIntegrityErrorFuture:
184190
"""result() raises TimeoutError first, then the recovered IntegrityError."""
185191

186192
def __init__(self) -> None:
187-
self._calls = 0
193+
self._done_checked = False
188194

189195
def cancel(self) -> bool:
190196
return False
191197

192198
def done(self) -> bool:
199+
# Only the recovery arm calls done(); see _LateSuccessFuture.
200+
self._done_checked = True
193201
return True
194202

195203
def cancelled(self) -> bool:
196204
return False
197205

198206
def result(self, timeout: float | None = None) -> Any:
199-
self._calls += 1
200-
if self._calls == 1:
201-
raise cf.TimeoutError()
202-
raise IntegrityError(
203-
"UNIQUE constraint failed", code=2067, raw_message="UNIQUE constraint failed"
204-
)
207+
if self._done_checked:
208+
raise IntegrityError(
209+
"UNIQUE constraint failed",
210+
code=2067,
211+
raw_message="UNIQUE constraint failed",
212+
)
213+
raise cf.TimeoutError()
205214

206215
stub = _LateIntegrityErrorFuture()
207216

@@ -235,22 +244,25 @@ def test_recovered_exception_carries_timeout_in_context(
235244

236245
class _Stub:
237246
def __init__(self) -> None:
238-
self._calls = 0
247+
self._done_checked = False
239248

240249
def cancel(self) -> bool:
241250
return False
242251

243252
def done(self) -> bool:
253+
# Recovery happens via the except-TimeoutError arm (which calls done()),
254+
# so the recovered IntegrityError's __context__ is the re-raised
255+
# TimeoutError; raising it during a slice would lose that context.
256+
self._done_checked = True
244257
return True
245258

246259
def cancelled(self) -> bool:
247260
return False
248261

249262
def result(self, timeout: float | None = None) -> Any:
250-
self._calls += 1
251-
if self._calls == 1:
252-
raise cf.TimeoutError()
253-
raise IntegrityError("constraint failed", code=2067, raw_message="x")
263+
if self._done_checked:
264+
raise IntegrityError("constraint failed", code=2067, raw_message="x")
265+
raise cf.TimeoutError()
254266

255267
stub = _Stub()
256268
monkeypatch.setattr(
@@ -295,9 +307,12 @@ def _invalidate(self, exc: BaseException | None = None) -> None:
295307

296308
class _StubFuture:
297309
def __init__(self) -> None:
298-
self._calls = 0
310+
self._cancel_called = False
299311

300312
def cancel(self) -> bool:
313+
# The except-TimeoutError arm calls cancel() before the bounded wait;
314+
# see the logs-unexpected-error stub above.
315+
self._cancel_called = True
301316
return True
302317

303318
def done(self) -> bool:
@@ -307,10 +322,9 @@ def cancelled(self) -> bool:
307322
return False
308323

309324
def result(self, timeout: float | None = None) -> None:
310-
self._calls += 1
311-
if self._calls == 1:
312-
raise cf.TimeoutError()
313-
raise cf.CancelledError() # bounded-wait: coroutine unwound cleanly
325+
if self._cancel_called:
326+
raise cf.CancelledError() # bounded-wait: coroutine unwound cleanly
327+
raise cf.TimeoutError()
314328

315329
stub = _StubFuture()
316330

0 commit comments

Comments
 (0)