signals/utils: always handle received signals#11361
Merged
glours merged 1 commit intodocker:mainfrom Jan 18, 2024
Merged
Conversation
laurazard
requested review from
glours and
jhrotko
and removed request for
a team
The changes in docker@dcbf005 fixed the "cancellable context" detection, and made it so that Compose would conditionally set up signal handling when the context was already not cancellable/when the plugin was running through the CLI, as we'd introduced a mechanism into the CLI to signal plugins to exit through a socket instead of handling signals themselves. This had some (not noticed at the time) issues when running through the CLI as, due to sharing a process group id with the parent CLI process, when a user CTRL-Cs the CLI will notify the plugin via the socket but the plugin process itself will also be signalled if attached to the TTY. This impacted some Compose commands that don't set up signal handling - so not `compose up`, but other commands would immediately quit instead of getting some "graceful" cancelled output. We initially attempted to address this "double notification" issue in the CLI by executing plugins under a new pgid so that they wouldn't be signalled, but that posed an issue with Buildx reading from the TTY, (see: moby/moby#47073) so we reverted the process group id changes and ended at a temporary solution in docker/cli#4792 where the CLI will only notify plugins via the socket when they are not already going to be signalled (when attached to a TTY). Due to this, plugins should always set up some signal handling, which this commit implements. Signed-off-by: Laura Brehm <laurabrehm@hey.com>
laurazard
force-pushed
the
always-handle-signals
branch
from
b90209e to
898e1b6
Compare
jhrotko
approved these changes
Jan 17, 2024
milas
approved these changes
Jan 17, 2024
| ctx, cancel := context.WithCancel(cmd.Context()) | ||
|
|
||
| s := make(chan os.Signal, 1) | ||
| signal.Notify(s, syscall.SIGTERM, syscall.SIGINT) |
Contributor
There was a problem hiding this comment.
fwiw I think this could be further simplified w/ signal.NotifyContext: https://pkg.go.dev/os/signal#NotifyContext
glours
approved these changes
Jan 17, 2024
Contributor
Author
|
Also sorry about all of these changes/reverts/fixes, CLI signal handling stuff is hard and sockets are not all created equal (did you know macOS does not support abstract sockets?) 🥲😭 |
Contributor
Author
|
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #11361 +/- ##
==========================================
+ Coverage 56.55% 56.57% +0.02%
==========================================
Files 136 136
Lines 11541 11538 -3
==========================================
+ Hits 6527 6528 +1
+ Misses 4387 4383 -4
Partials 627 627 ☔ View full report in Codecov by Sentry. |
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
4 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.
The changes in dcbf005 fixed the "cancellable context" detection, and made it so that Compose would conditionally set up signal handling when the context was already not cancellable/when the plugin was running through the CLI, as we'd introduced a mechanism into the CLI to signal plugins to exit through a socket instead of handling signals themselves.
This had some (not noticed at the time) issues when running through the CLI as, due to sharing a process group id with the parent CLI process, when a user CTRL-Cs the CLI will notify the plugin via the socket but the plugin process itself will also be signalled if attached to the TTY. This impacted some Compose commands that don't set up signal handling - so not
compose up, but other commands would immediately quit instead of getting some "graceful" cancelled output.We initially attempted to address this "double notification" issue in the CLI by executing plugins under a new pgid so that they wouldn't be signalled, but that posed an issue with Buildx reading from the TTY, (see: moby/moby#47073) so we reverted the process group id changes and ended at a temporary solution in docker/cli#4792 where the CLI will only notify plugins via the socket when they are not already going to be signalled (when attached to a TTY).
Due to this, plugins should always set up some signal handling, which this commit implements.
What I did
Make Compose always handle signals – you can verify the broken/fixed behaviour by building Compose from this PR and the CLI from master, and then trying:
docker compose pulland hitting CTRL-C before it finishesPre-changes:
after:
Related issue
(not mandatory) A picture of a cute animal, if possible in relation to what you did