Skip to content

Commit fff164c

Browse files
cpuguy83cpuguy83
committed
Ignore SIGURG on Linux.
In go1.14+, SIGURG is used by the runtime to handle preemtable system calls. In practice this signal caught *frequently*. For reference: https://go.googlesource.com/proposal/+/master/design/24543-non-cooperative-preemption.md golang/go#37942 Signed-off-by: Brian Goff <[email protected]>
1 parent 9a3fdc1 commit fff164c

10 files changed

Lines changed: 196 additions & 32 deletions

File tree

‎cli/command/container/attach.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,8 @@ func runAttach(dockerCli command.Cli, opts *attachOptions) error {
9797
}
9898

9999
if opts.proxy && !c.Config.Tty {
100-
sigc := ForwardAllSignals(ctx, dockerCli, opts.container)
100+
sigc := notfiyAllSignals()
101+
go ForwardAllSignals(ctx, dockerCli, opts.container, sigc)
101102
defer signal.StopCatch(sigc)
102103
}
103104

‎cli/command/container/client_test.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ type fakeClient struct {
3232
containerExportFunc func(string) (io.ReadCloser, error)
3333
containerExecResizeFunc func(id string, options types.ResizeOptions) error
3434
containerRemoveFunc func(ctx context.Context, container string, options types.ContainerRemoveOptions) error
35+
containerKillFunc func(ctx context.Context, container, signal string) error
3536
Version string
3637
}
3738

@@ -154,3 +155,10 @@ func (f *fakeClient) ContainerExecResize(_ context.Context, id string, options t
154155
}
155156
return nil
156157
}
158+
159+
func (f *fakeClient) ContainerKill(ctx context.Context, container, signal string) error {
160+
if f.containerKillFunc != nil {
161+
return f.containerKillFunc(ctx, container, signal)
162+
}
163+
return nil
164+
}

‎cli/command/container/run.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,8 @@ func runContainer(dockerCli command.Cli, opts *runOptions, copts *containerOptio
131131
return runStartContainerErr(err)
132132
}
133133
if opts.sigProxy {
134-
sigc := ForwardAllSignals(ctx, dockerCli, createResponse.ID)
134+
sigc := notfiyAllSignals()
135+
go ForwardAllSignals(ctx, dockerCli, createResponse.ID, sigc)
135136
defer signal.StopCatch(sigc)
136137
}
137138

‎cli/command/container/signals.go‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
package container
2+
3+
import (
4+
"context"
5+
"fmt"
6+
"os"
7+
gosignal "os/signal"
8+
9+
"github.com/docker/cli/cli/command"
10+
"github.com/docker/docker/pkg/signal"
11+
"github.com/sirupsen/logrus"
12+
)
13+
14+
// ForwardAllSignals forwards signals to the container
15+
//
16+
// The channel you pass in must already be setup to receive any signals you want to forward.
17+
func ForwardAllSignals(ctx context.Context, cli command.Cli, cid string, sigc <-chan os.Signal) {
18+
var s os.Signal
19+
for {
20+
select {
21+
case s = <-sigc:
22+
case <-ctx.Done():
23+
return
24+
}
25+
26+
if s == signal.SIGCHLD || s == signal.SIGPIPE {
27+
continue
28+
}
29+
30+
// In go1.14+, the go runtime issues SIGURG as an interrupt to support pre-emptable system calls on Linux.
31+
// Since we can't forward that along we'll check that here.
32+
if isRuntimeSig(s) {
33+
continue
34+
}
35+
var sig string
36+
for sigStr, sigN := range signal.SignalMap {
37+
if sigN == s {
38+
sig = sigStr
39+
break
40+
}
41+
}
42+
if sig == "" {
43+
fmt.Fprintf(cli.Err(), "Unsupported signal: %v. Discarding.\n", s)
44+
continue
45+
}
46+
47+
if err := cli.Client().ContainerKill(ctx, cid, sig); err != nil {
48+
logrus.Debugf("Error sending signal: %s", err)
49+
}
50+
}
51+
}
52+
53+
func notfiyAllSignals() chan os.Signal {
54+
sigc := make(chan os.Signal, 128)
55+
gosignal.Notify(sigc)
56+
return sigc
57+
}

‎cli/command/container/signals_linux.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
package container
2+
3+
import (
4+
"os"
5+
6+
"golang.org/x/sys/unix"
7+
)
8+
9+
func isRuntimeSig(s os.Signal) bool {
10+
return s == unix.SIGURG
11+
}

‎cli/command/container/signals_linux_test.go‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
package container
2+
3+
import (
4+
"context"
5+
"os"
6+
"syscall"
7+
"testing"
8+
"time"
9+
10+
"github.com/docker/cli/internal/test"
11+
"golang.org/x/sys/unix"
12+
"gotest.tools/v3/assert"
13+
)
14+
15+
func TestIgnoredSignals(t *testing.T) {
16+
ignoredSignals := []syscall.Signal{unix.SIGPIPE, unix.SIGCHLD, unix.SIGURG}
17+
18+
for _, s := range ignoredSignals {
19+
t.Run(unix.SignalName(s), func(t *testing.T) {
20+
ctx, cancel := context.WithCancel(context.Background())
21+
defer cancel()
22+
23+
var called bool
24+
client := &fakeClient{containerKillFunc: func(ctx context.Context, container, signal string) error {
25+
called = true
26+
return nil
27+
}}
28+
29+
cli := test.NewFakeCli(client)
30+
sigc := make(chan os.Signal)
31+
defer close(sigc)
32+
33+
done := make(chan struct{})
34+
go func() {
35+
ForwardAllSignals(ctx, cli, t.Name(), sigc)
36+
close(done)
37+
}()
38+
39+
timer := time.NewTimer(30 * time.Second)
40+
defer timer.Stop()
41+
42+
select {
43+
case <-timer.C:
44+
t.Fatal("timeout waiting to send signal")
45+
case sigc <- s:
46+
case <-done:
47+
}
48+
49+
// cancel the context so ForwardAllSignals will exit after it has processed the signal we sent.
50+
// This is how we know the signal was actually processed and are not introducing a flakey test.
51+
cancel()
52+
<-done
53+
54+
assert.Assert(t, !called, "kill was called")
55+
})
56+
}
57+
}

‎cli/command/container/signals_notlinux.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
// +build !linux
2+
3+
package container
4+
5+
import "os"
6+
7+
func isRuntimeSig(_ os.Signal) bool {
8+
return false
9+
}

‎cli/command/container/signals_test.go‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
package container
2+
3+
import (
4+
"context"
5+
"os"
6+
"testing"
7+
"time"
8+
9+
"github.com/docker/cli/internal/test"
10+
"github.com/docker/docker/pkg/signal"
11+
)
12+
13+
func TestForwardSignals(t *testing.T) {
14+
ctx, cancel := context.WithCancel(context.Background())
15+
defer cancel()
16+
17+
called := make(chan struct{})
18+
client := &fakeClient{containerKillFunc: func(ctx context.Context, container, signal string) error {
19+
close(called)
20+
return nil
21+
}}
22+
23+
cli := test.NewFakeCli(client)
24+
sigc := make(chan os.Signal)
25+
defer close(sigc)
26+
27+
go ForwardAllSignals(ctx, cli, t.Name(), sigc)
28+
29+
timer := time.NewTimer(30 * time.Second)
30+
defer timer.Stop()
31+
32+
select {
33+
case <-timer.C:
34+
t.Fatal("timeout waiting to send signal")
35+
case sigc <- signal.SignalMap["TERM"]:
36+
}
37+
if !timer.Stop() {
38+
<-timer.C
39+
}
40+
timer.Reset(30 * time.Second)
41+
42+
select {
43+
case <-called:
44+
case <-timer.C:
45+
t.Fatal("timeout waiting for signal to be processed")
46+
}
47+
48+
}

‎cli/command/container/start.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,8 @@ func runStart(dockerCli command.Cli, opts *startOptions) error {
7474

7575
// We always use c.ID instead of container to maintain consistency during `docker start`
7676
if !c.Config.Tty {
77-
sigc := ForwardAllSignals(ctx, dockerCli, c.ID)
77+
sigc := notfiyAllSignals()
78+
ForwardAllSignals(ctx, dockerCli, c.ID, sigc)
7879
defer signal.StopCatch(sigc)
7980
}
8081

‎cli/command/container/tty.go‎

Lines changed: 0 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -95,32 +95,3 @@ func MonitorTtySize(ctx context.Context, cli command.Cli, id string, isExec bool
9595
}
9696
return nil
9797
}
98-
99-
// ForwardAllSignals forwards signals to the container
100-
func ForwardAllSignals(ctx context.Context, cli command.Cli, cid string) chan os.Signal {
101-
sigc := make(chan os.Signal, 128)
102-
signal.CatchAll(sigc)
103-
go func() {
104-
for s := range sigc {
105-
if s == signal.SIGCHLD || s == signal.SIGPIPE {
106-
continue
107-
}
108-
var sig string
109-
for sigStr, sigN := range signal.SignalMap {
110-
if sigN == s {
111-
sig = sigStr
112-
break
113-
}
114-
}
115-
if sig == "" {
116-
fmt.Fprintf(cli.Err(), "Unsupported signal: %v. Discarding.\n", s)
117-
continue
118-
}
119-
120-
if err := cli.Client().ContainerKill(ctx, cid, sig); err != nil {
121-
logrus.Debugf("Error sending signal: %s", err)
122-
}
123-
}
124-
}()
125-
return sigc
126-
}

0 commit comments

Comments
 (0)