fix: on Ctrl+C, stop accepting writes and flush what is already enqueued - #321
Merged
Conversation
The SIGINT handler called `cancel`, whose contract was to abandon buffered points and exit fast.
The behaviour we want is the opposite: refuse further writes, let everything already enqueued
reach avro or the backend, and exit once it has.
Refusing first is what makes the drain converge. Without it the teardown races a producer loop
that has not noticed the interrupt yet, and on a multi-threaded writer it never notices at all --
KeyboardInterrupt is delivered only to the main thread.
`stop_accepting_writes` takes `&self` rather than `&mut self` deliberately: shutdown starts on
whichever thread caught the signal while other threads may be inside `enqueue`, and `close` needs
`&mut self`, which would fail to borrow in that situation. The handler sets the flag and re-raises
KeyboardInterrupt; the drain then happens in `close`, reached through `__exit__` as the exception
unwinds, or through the stream's destructor if the caller is not using a context manager.
Writes attempted after shutdown begins raise `RuntimeError("stream is shutting down")`.
Verified by sending a real SIGINT to a streaming process:
single writer: 10,442,916 points enqueued, 10,442,916 in the avro file, exited in 3.4s
four writers: 1,498,653 points enqueued, 1,498,653 in the avro file, all 4 threads refused
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Folded into #320. These are not safe to merge separately: with only the perf commit, Ctrl+C on a multi-threaded writer deadlocks. |
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.
Stacked on #320. Touches only
py-nominal-streaming.Desired behaviour
On Ctrl+C: no further data can be enqueued, everything already enqueued reaches avro or the
backend, and the process exits once it has.
What it did instead
The handler called
cancel, whose contract was to abandon buffered points for a fast exit --the opposite of the above.
What changes
stop_accepting_writessets a flag that makes subsequent writes raiseRuntimeError("stream is shutting down"). The SIGINT handler sets it and re-raisesKeyboardInterrupt; the drain then happens in the ordinary
close, reached through__exit__asthe exception unwinds, or through the stream's destructor (added in #320) otherwise.
Refusing first is what makes the drain converge, rather than racing a producer that has not
noticed the interrupt. That matters most for multi-threaded writers: KeyboardInterrupt is
delivered only to the main thread, so worker threads would otherwise keep feeding the buffer.
They now see the
RuntimeErrorinstead.stop_accepting_writestakes&self, not&mut self, on purpose: shutdown begins on whicheverthread caught the signal while others may be inside
enqueue, andcloseneeds&mut self,which would fail to borrow in that situation.
Verification
Real SIGINT sent to a live streaming process, avro file read back and compared against the count
the process reported enqueuing:
Note on the earlier approach
#318 implemented the opposite semantics (abandon the backlog to exit immediately) against the
core crate. It should be closed in favour of this unless a fast-and-lossy teardown is separately
wanted -- and if it is, it belongs behind an explicit call rather than on Ctrl+C.
🤖 Generated with Claude Code