Skip to content

Commit

Permalink
attach: don't return context cancelled error
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
laurazard committed Jul 24, 2024
1 parent 1e0f669 commit 66aa0f6
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 1 deletion.
6 changes: 5 additions & 1 deletion cli/command/container/attach.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,8 @@ func RunAttach(ctx context.Context, dockerCLI command.Cli, containerID string, o
detachKeys: options.DetachKeys,
}

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

Expand All @@ -162,6 +163,9 @@ func getExitStatus(errC <-chan error, resultC <-chan container.WaitResponse) err
return cli.StatusError{StatusCode: int(result.StatusCode)}
}
case err := <-errC:
if errors.Is(err, context.Canceled) {
return nil
}
return err
}

Expand Down
5 changes: 5 additions & 0 deletions cli/command/container/attach_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package container

import (
"context"
"io"
"testing"

Expand Down Expand Up @@ -117,6 +118,10 @@ func TestGetExitStatus(t *testing.T) {
},
expectedError: cli.StatusError{StatusCode: 15},
},
{
err: context.Canceled,
expectedError: nil,
},
}

for _, testcase := range testcases {
Expand Down
29 changes: 29 additions & 0 deletions e2e/container/attach_test.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
package container

import (
"bytes"
"fmt"
"os"
"os/exec"
"strings"
"testing"
"time"

"github.com/creack/pty"
"github.com/docker/cli/e2e/internal/fixtures"
"gotest.tools/v3/assert"
"gotest.tools/v3/icmd"
)

Expand All @@ -23,3 +29,26 @@ func TestAttachExitCode(t *testing.T) {
func withStdinNewline(cmd *icmd.Cmd) {
cmd.Stdin = strings.NewReader("\n")
}

// Regression test for https://github.com/docker/cli/issues/5294
func TestAttachInterrupt(t *testing.T) {
result := icmd.RunCommand("docker", "run", "-d", fixtures.AlpineImage, "sh", "-c", "sleep 5")
result.Assert(t, icmd.Success)
containerID := strings.TrimSpace(result.Stdout())

// run it as such so we can signal it later
c := exec.Command("docker", "attach", containerID)
d := bytes.Buffer{}
c.Stdout = &d
c.Stderr = &d
_, err := pty.Start(c)
assert.NilError(t, err)

// have to wait a bit to give time for the command to execute/print
time.Sleep(500 * time.Millisecond)
c.Process.Signal(os.Interrupt)

_ = c.Wait()
assert.Equal(t, c.ProcessState.ExitCode(), 0)
assert.Equal(t, d.String(), "")
}

0 comments on commit 66aa0f6

Please sign in to comment.