fix: give each request its own Signals instead of the process-wide one - #54
Open
cablehead wants to merge 1 commit into
Open
fix: give each request its own Signals instead of the process-wide one#54cablehead wants to merge 1 commit into
cablehead wants to merge 1 commit into
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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