Skip to content

Commit 53433e1

Browse files
committed
attach: don't return context cancelled error
In 3f0d90a we introduced a global signal handler and made sure all the contexts passed into command execution get (appropriately) cancelled when we get a SIGINT. Due to that change, and how we use this context during `docker attach`, we started to return the context cancelation error when a user signals the running `docker attach`. Since this is the intended behavior, we shouldn't return an error, so this commit adds checks to ignore this specific error in this case. Also adds a regression test. Signed-off-by: Laura Brehm <laurabrehm@hey.com>
1 parent 1e0f669 commit 53433e1

3 files changed

Lines changed: 40 additions & 1 deletion

File tree

cli/command/container/attach.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"github.com/docker/cli/cli"
88
"github.com/docker/cli/cli/command"
99
"github.com/docker/cli/cli/command/completion"
10+
"github.com/docker/docker/api/types"
1011
"github.com/docker/docker/api/types/container"
1112
"github.com/docker/docker/client"
1213
"github.com/moby/sys/signal"
@@ -145,7 +146,8 @@ func RunAttach(ctx context.Context, dockerCLI command.Cli, containerID string, o
145146
detachKeys: options.DetachKeys,
146147
}
147148

148-
if err := streamer.stream(ctx); err != nil {
149+
// if the context was canceled, this was likely intentional and we shouldn't return an error
150+
if err := streamer.stream(ctx); err != nil && !errors.Is(err, context.Canceled) {
149151
return err
150152
}
151153

@@ -162,6 +164,9 @@ func getExitStatus(errC <-chan error, resultC <-chan container.WaitResponse) err
162164
return cli.StatusError{StatusCode: int(result.StatusCode)}
163165
}
164166
case err := <-errC:
167+
if errors.Is(err, context.Canceled) {
168+
return nil
169+
}
165170
return err
166171
}
167172

cli/command/container/attach_test.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package container
22

33
import (
4+
"context"
45
"io"
56
"testing"
67

@@ -117,6 +118,10 @@ func TestGetExitStatus(t *testing.T) {
117118
},
118119
expectedError: cli.StatusError{StatusCode: 15},
119120
},
121+
{
122+
err: context.Canceled,
123+
expectedError: nil,
124+
},
120125
}
121126

122127
for _, testcase := range testcases {

e2e/container/attach_test.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,17 @@
11
package container
22

33
import (
4+
"bytes"
45
"fmt"
6+
"os"
7+
"os/exec"
58
"strings"
69
"testing"
10+
"time"
711

12+
"github.com/creack/pty"
813
"github.com/docker/cli/e2e/internal/fixtures"
14+
"gotest.tools/v3/assert"
915
"gotest.tools/v3/icmd"
1016
)
1117

@@ -23,3 +29,26 @@ func TestAttachExitCode(t *testing.T) {
2329
func withStdinNewline(cmd *icmd.Cmd) {
2430
cmd.Stdin = strings.NewReader("\n")
2531
}
32+
33+
// Regression test for https://github.com/docker/cli/issues/5294
34+
func TestAttachInterrupt(t *testing.T) {
35+
result := icmd.RunCommand("docker", "run", "-d", fixtures.AlpineImage, "sh", "-c", "sleep 5")
36+
result.Assert(t, icmd.Success)
37+
containerID := strings.TrimSpace(result.Stdout())
38+
39+
// run it as such so we can signal it later
40+
c := exec.Command("docker", "attach", containerID)
41+
d := bytes.Buffer{}
42+
c.Stdout = &d
43+
c.Stderr = &d
44+
_, err := pty.Start(c)
45+
assert.NilError(t, err)
46+
47+
// have to wait a bit to give time for the command to execute/print
48+
time.Sleep(500 * time.Millisecond)
49+
c.Process.Signal(os.Interrupt)
50+
51+
_ = c.Wait()
52+
assert.Equal(t, c.ProcessState.ExitCode(), 0)
53+
assert.Equal(t, d.String(), "")
54+
}

0 commit comments

Comments
 (0)