Fix ctrl-c not propagating to the containers for a 'docker run' on Windows

[mat007]

- What I did

I fixed docker run hanging after a ctrl-c on Windows.

- How I did it

I bumped the vendoring to moby/sys to add many more signal definitions for Windows, including TERM.

- How to verify it

Running docker run nginx and hitting ctrl-c was hanging before the fix.
After the fix it exits and a docker ps -a shows the container has been exited.

- Description for the changelog

Fixed ctrl-c hanging on Windows to exit after running a container in non-interactive mode.

- A picture of a cute animal (not mandatory but encouraged)

(sorry if this was posted before, I’m new at this)

[mat007]

I’m not sure what the comment inside the brackets means 😬
Also maybe we’d want a new released version of moby/sys eventually?

[thaJeztah]

The github.com/moby/sys repository is a collection of modules, and each is versioned separately. So when using go modules, you need to use one (or more) of;

• github.com/moby/sys/mount@<version>
• github.com/moby/sys/mountinfo@<version>
• github.com/moby/sys/signal@<version>
• github.com/moby/sys/symlink@<version>

The cli doesn't yet use go modules, so we're vendoring the "root" of moby/sys, but want to match a tagged release if possible (so that we would match whatever someone would be using when using go modules).

I think there may be one or two changes left to make in the github.com/moby/sys/signal module, and after that we can do a new release.

Let me have a look at that, and perhaps we can have those in before we merge this.

Note that the 20.10 branch does not yet use github.com/moby/sys/signal, but uses the same package (which used to be in https://github.com/moby/moby, so if we need this for the docker 20.10 cli, we may need to either update the package in moby/moby, or backport the migration to the new package 😓

[mat007]

@thaJeztah anything I can do to help?

[mat007]

I think there may be one or two changes left to make in the github.com/moby/sys/signal module

Any details there? I don’t see any related pending issue or PR in that repo.

[thaJeztah]

It took a bit longer, as some fixes for k8s were pending, but I just tagged signal v0.6.0: https://github.com/moby/sys/releases/tag/signal%2Fv0.6.0

Can you change this to:

Suggested change

	github.com/moby/sys 76a42cd566e5f05c3155179ea779e418df4ff4f9 # master (either mount/vXXX, mountinfo/vXXX, signal/vXXX, or symlink/vXXX)
	github.com/moby/sys 03b9f8d59a07f5206a2264105f4903a222aea964 # signal/v0.6.0 (latest tag, either mount/vXXX, mountinfo/vXXX, signal/vXXX, or symlink/vXXX)

And re-vendor? (please squash the commits if needed)

[mat007]

Done!

[codecov-commenter]

Codecov Report

Merging #3302 (feb27af) into master (fe2008d) will not change coverage.
The diff coverage is 60.00%.

@@           Coverage Diff           @@
##           master    #3302   +/-   ##
=======================================
  Coverage   57.99%   57.99%           
=======================================
  Files         302      302           
  Lines       21764    21764           
=======================================
  Hits        12621    12621           
  Misses       8219     8219           
  Partials      924      924           

[mat007]

This PR doesn’t seem to fix all cases, for instance the one described in docker-archive/compose-cli#1151 (comment) doesn’t work on Windows (unless -it).

I think the problem has to do with using STOPSIGNAL in the Dockerfile:

STOPSIGNAL SIGINT

The signal does go through the ForwardAllSignals function and calls

		if err := cli.Client().ContainerKill(ctx, cid, sig); err != nil {

with sig being INT.

I’m not sure how to track further, given it works properly on Mac. 🤔

[mat007]

Oh wait, it actually works fine using cmd and powershell.
Only my msys based terminal has that issue… 🤔

Prefixing with winpty in msys makes it work as well…

[mat007]

@thaJeztah can I help unblocking this PR in some way?

[rfay]

Also, shouldn't SIGINT be presented to the container's main process and the behavior be generated from there? And some processes (like mysql) don't care, want other signals.

[thaJeztah]

LGTM