#40353 passes with this change at least, so moving out of draft 😅

I don't see what race this is fixing.

For that matter sending the signal should be async anyway. I'm not sure we need the goroutine.

Perhaps I'm completely off, but signal was used in the go func and could be pointing to a different value after it's started, but you're right that this probably doesn't solve the issue

signal is defined inside the loop, therefore each loop iteration gets a new definition.
If it was var signal string outside of the loop (or, for instance the goroutine was using sigStr) then it would be overwritten in the goroutine.

In any case, I think this test will work without the goroutine (and no need for sleep).

or, for instance the goroutine was using sigStr

you're right; I was thinking about that case, but it's not the problem. I can update to remove the goroutine 👍

LOL; interesting; looks like Goland runs tests in an interesting way for this file, and adds both linux and darwin build-tags;

GOROOT=/usr/local/Cellar/go/1.13/libexec #gosetup
GOPATH=/Users/sebastiaan/go #gosetup
/usr/local/Cellar/go/1.13/libexec/bin/go test -c -tags "darwin linux" -o /private/var/folders/c_/vjh56sc12fd2b_q2n02_lt140000gn/T/___TestCatchAll_in_github_com_docker_docker_pkg_signal github.com/docker/docker/pkg/signal #gosetup
# runtime/internal/sys
/usr/local/Cellar/go/1.13/libexec/src/runtime/internal/sys/zgoos_linux.go:8:7: GOOS redeclared in this block
	previous declaration at /usr/local/Cellar/go/1.13/libexec/src/runtime/internal/sys/zgoos_darwin.go:7:14

Updated 👍

@AkihiroSuda @tiborvass LGTY?

ping @tiborvass @kolyshkin PTAL

I was just looking into this code today (not the tests). These functions (together with their test cases) should be moved to docker/cli, as they are only used for cli.

I'm building the docker branch 19.03 against golang 1.14.2, and I still observe this test failure after applying this patch:

=== RUN   TestCatchAll
    TestCatchAll: signal_linux_test.go:32: assertion failed: urgent I/O condition (string) != hangup (string)
    TestCatchAll: signal_linux_test.go:32: assertion failed: hangup (string) != child exited (string)
    TestCatchAll: signal_linux_test.go:32: assertion failed: child exited (string) != illegal instruction (string)
    TestCatchAll: signal_linux_test.go:32: assertion failed: illegal instruction (string) != floating point exception (string)
    TestCatchAll: signal_linux_test.go:32: assertion failed: floating point exception (string) != child exited (string)
--- FAIL: TestCatchAll (0.00s)

Is the patch supposed to address this issue in the end?

(if the answer is no, you might want to update the name of the issue at #40353 (comment))

This is only in the master branch, not in the 19.03 branch. Neither branches currently work with Go 1.14 (and there's still known issues in containerd and runc as well with compatibility with Go 1.14); Id recommend sticking to Go 1.13 to build for now

Thanks for the quick reply!

This is only in the master branch, not in the 19.03 branch

Yep sure, what I meant is that I observe this test failure on the 19.03 branch, and applying this patch doesn't fix it. It looks like the original intention of this PR was to fix a race, but then it became a maintenance commit, so I just wanted to clarify that the test failure persists after this commit.

Ah, gotcha, thanks! I haven't continued to look at Go 1.14 compatibility (because I know some "lower level" projects (containerd, runc) still had to work on some fixes and it didn't have the highest priority.