fix bad SIGINT patterns - #689
Merged
plebhash merged 3 commits intoAug 10, 2026
Merged
Conversation
Member
Author
|
the scope of this PR (more specifically the top commit, currently hashed as fc3cfc0) overlaps with:
but let's see what gets merged first, and then rebase accordingly... no strong opinions on the specific order |
This comment was marked as resolved.
This comment was marked as resolved.
bit-aloo
reviewed
Aug 7, 2026
bit-aloo
left a comment
bit-aloo
left a comment
Member
There was a problem hiding this comment.
Changes makes sense. Some suggestions and questions.
plebhash
force-pushed
the
2026-08-05-noise-connection-ctrl-c
branch
from
August 7, 2026 20:27
fc3cfc0 to
3c7f7ae
Compare
This comment was marked as resolved.
This comment was marked as resolved.
…onnection `Connection::spawn_reader` and `Connection::spawn_writer` both selected on `tokio::signal::ctrl_c()` as their shutdown arm. The first poll of either future makes tokio install a process-global SIGINT handler, so from that point on SIGINT no longer terminates the process, it only completes those two futures. Any binary opening a single noise Connection silently lost default Ctrl+C behavior, and could not be interrupted if it later hung. Signal handling belongs to the application, not to a transport helper. Replace both arms with a caller-supplied CancellationToken, mirroring the sibling helper `ConnectionSV1::new`. `tokio_util` is re-exported from the crate root so consumers can build the token without declaring their own dependency on it. Note this is a breaking change to `Connection::new`.
The sniffers selected on `tokio::signal::ctrl_c()` in arms that do nothing. Their only real effect was installing a process-global SIGINT handler, which stops SIGINT from terminating the test binary. Drop those arms so the sniffers no longer claim the signal. In `wait_for_message` and `wait_for_keepalive_notify` this leaves a single-arm `select!`, so the remaining block is awaited directly. Note this is not sufficient on its own to make Ctrl+C kill a hung test: any test that starts a role in-process still installs that role's own SIGINT handler (e.g. `pool_runtime::wait_for_shutdown`), which is correct behavior for the role binaries but leaks into the test process.
…ries `PoolRuntime::wait_for_shutdown`, `TranslatorSv2::start` and `JobDeclaratorClient::start` each selected on `tokio::signal::ctrl_c()`. That made the libraries install a process-global SIGINT handler, so any process embedding a role lost default Ctrl+C behavior: in the integration tests, a SIGINT would gracefully stop the role but leave the test harness running, making a hung test impossible to interrupt. All three run loops already break on their own CancellationToken, and all three types expose `shutdown()`, so the ctrl_c arms are removed and each binary now watches for the signal itself and calls `shutdown()`. Behavior of the binaries is unchanged: Ctrl+C still triggers the same graceful shutdown path, it is just initiated by the binary rather than by the library.
plebhash
force-pushed
the
2026-08-05-noise-connection-ctrl-c
branch
from
August 10, 2026 15:48
3c7f7ae to
7992938
Compare
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
2 participants
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.
close #688