Skip to content

forward signal to container#10633

Merged
ndeloof merged 1 commit intodocker:v2from
ndeloof:run_signal
May 31, 2023
Merged

forward signal to container#10633
ndeloof merged 1 commit intodocker:v2from
ndeloof:run_signal

Conversation

@ndeloof
Copy link
Contributor

@ndeloof ndeloof commented May 30, 2023

What I did
as compose run a container, forward signals to container the same way docker run does
we have to because RunStart only doest it for container without a TTY (https://github.com/docker/cli/blob/master/cli/command/container/start.go#L91)

As container is ran interactive from a terminal (--tty=true), RunStart uses setRawTerminal and doing so user to hi Ctrl+C just directly send those keys to target process, not compose command itself.
But when signal is sent to compose by a kill command it has to handle this and forward signal explicitly, just like RunStart does when container has no TTY.

Related issue
fixes #10586

(not mandatory) A picture of a cute animal, if possible in relation to what you did

@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.10 🎉

Comparison is base (b0a35cc) 59.40% compared to head (6b91f4a) 59.51%.

Additional details and impacted files
@@            Coverage Diff             @@
##               v2   #10633      +/-   ##
==========================================
+ Coverage   59.40%   59.51%   +0.10%     
==========================================
  Files         107      107              
  Lines        9395     9403       +8     
==========================================
+ Hits         5581     5596      +15     
+ Misses       3240     3234       -6     
+ Partials      574      573       -1     
Impacted Files Coverage Δ
pkg/compose/run.go 57.73% <100.00%> (+3.79%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ndeloof ndeloof marked this pull request as ready for review May 30, 2023 13:24
Signed-off-by: Nicolas De Loof <[email protected]>
Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ndeloof ndeloof merged commit fed8ef6 into docker:v2 May 31, 2023
@ndeloof ndeloof deleted the run_signal branch May 31, 2023 13:10
@g0t4
Copy link
Contributor

g0t4 commented Nov 1, 2023

Would this fix allow for Ctrl+R to be sent to a container started with docker compose up? I'm trying to find if there are any open issues with regards to stdin and docker compose managed containers.

@ndeloof
Copy link
Contributor Author

ndeloof commented Nov 2, 2023

@g0t4 docker compose up is not interactive, it doesn't attach to a specific container, and as such won't forward signals to service(s). You can use docker compose kill -s [SIGNAL] SERVICE for this purpose

@kuro337
Copy link

kuro337 commented Mar 16, 2024

@ndeloof Hello sir, is this still true?

If a process is launched with docker compose up - there is no way for my image to be sent the shutdown signal?

Are there any ways to achieve it?

@ndeloof
Copy link
Contributor Author

ndeloof commented Mar 16, 2024

Ctrl+C or down command will send a SIGINT signal to your container, you can tweak signal being sent and timeout waiting for termination in your compose.yaml file, see https://github.com/compose-spec/compose-spec/blob/master/05-services.md#stop_grace_period

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.

[BUG] compose run doesn't forward signals

4 participants