Skip to content

fix: give each request its own Signals instead of the process-wide one - #54

Open
cablehead wants to merge 1 commit into
mainfrom
feat/per-connection-signals
Open

fix: give each request its own Signals instead of the process-wide one#54
cablehead wants to merge 1 commit into
mainfrom
feat/per-connection-signals

Conversation

@cablehead

Copy link
Copy Markdown
Owner

Every request's ThreadJob in spawn_eval_thread was built from
engine.state.signals().clone(), a clone of the process-wide interrupt.
ThreadJob::kill() triggers that signal, so killing one job would trip
every concurrent request's signal. Nothing called kill on disconnect
either: the only kill_and_remove call site is the Ctrl-C handler in
main.rs, which kills every job in the table at once. A client that
disconnected mid-stream left its job in the table and its thread
parked in whatever blocking read the closure was in, for the life of
the server.

Each request now gets a fresh Arc/Signals, set on both the
ThreadJob and the engine state the closure runs on. A watcher spawned
alongside the response channel, in both the ListStream and ByteStream
branches, waits on the channel sender's closed() and calls job.kill()
once there is no receiver left, whether the client disconnected or the
response finished normally. Ctrl-C still kills every job in the table,
one kill() call per job, each now tripping its own request's signal
instead of the shared one.

Tests model an idle producer with .bus sub on an unpublished topic
rather than a timer-driven generator. A generator that produces on its
own discovers a dropped receiver through the existing tx.send failure
regardless of this fix, which hides the leak the same way live traffic
hides it on a real server.

Measured against a real xs store with a .cat --follow handler: before
this fix, 10 abandoned SSE connections left 10 threads parked and the
job table growing without bound; after, thread count returns to
baseline once each connection's TCP socket is actually torn down.

Claude-Session: https://claude.ai/code/session_01CDDhWYJoyN9ENGEG1zhyBh

spawn_eval_thread built every request's ThreadJob from
engine.state.signals().clone(), a clone of the process-wide interrupt.
ThreadJob::kill() triggers that signal, so killing any one job would
have tripped every concurrent request. Nothing called kill on
disconnect either: the only kill_and_remove call site is the Ctrl-C
handler in main.rs, which kills every job in the table at once. A
client that disconnected mid-stream left its job in the table and its
thread parked in whatever blocking read the closure was in, for the
life of the server.

Each request now gets a fresh Arc<AtomicBool>/Signals, set on both the
ThreadJob and the engine state the closure runs on. A watcher spawned
alongside the response channel in both the ListStream and ByteStream
branches waits on the channel sender's closed() and calls job.kill()
once there is no receiver left, whether because the client disconnected
or the response finished normally. Ctrl-C is unaffected: it still kills
every job in the table, one kill() call per job, each tripping its own
request's signal now instead of the shared one.

Tests model an idle producer with .bus sub on an unpublished topic
rather than a timer-driven generator: a generator that produces on its
own discovers a dropped receiver via the existing tx.send failure
regardless of this fix, hiding the leak the same way live traffic does
on the real reproduction harness.

Claude-Session: https://claude.ai/code/session_01CDDhWYJoyN9ENGEG1zhyBh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants