From 30c4637f03ecb8855fe6e47f5374c89d50222577 Mon Sep 17 00:00:00 2001 From: Laura Brehm Date: Mon, 2 Dec 2024 10:33:17 +0000 Subject: [PATCH] run: don't hang if only attaching STDIN MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If STDOUT or STDERR are attached and the container exits, the streams will be closed by the daemon while the container is exiting, causing the streamer to return an error https://github.com/docker/cli/blob/61b02e636d2afed778f2e871c3ec1c6adee69ca4/cli/command/container/hijack.go#L53 that gets sent https://github.com/docker/cli/blob/61b02e636d2afed778f2e871c3ec1c6adee69ca4/cli/command/container/run.go#L278 and received https://github.com/docker/cli/blob/61b02e636d2afed778f2e871c3ec1c6adee69ca4/cli/command/container/run.go#L225 on `errCh`. However, if only STDIN is attached, it's not closed (since this is attached to the user's TTY) when the container exits, so the streamer doesn't exit and nothing gets sent on `errCh`, meaning the CLI execution hangs receiving on `errCh` on L231. Change the logic to receive on both `errCh` and `statusChan` – this way, if the container exits, we get notified on `statusChan` (even if only STDIN is attached), and can cancel the streamer and exit. Signed-off-by: Laura Brehm --- cli/command/container/run.go | 33 ++++++++++++++++++++++----------- e2e/container/run_test.go | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 11 deletions(-) diff --git a/cli/command/container/run.go b/cli/command/container/run.go index ed4c96574c..5ef3e8b7bf 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -228,20 +228,31 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption } } - if err := <-errCh; err != nil { - if _, ok := err.(term.EscapeError); ok { - // The user entered the detach escape sequence. - return nil + select { + case err := <-errCh: + if err != nil { + if _, ok := err.(term.EscapeError); ok { + // The user entered the detach escape sequence. + return nil + } + + logrus.Debugf("Error hijack: %s", err) + return err + } + status := <-statusChan + if status != 0 { + return cli.StatusError{StatusCode: status} + } + case status := <-statusChan: + // notify hijackedIOStreamer that we're exiting and wait + // so that the terminal can be restored. + cancelFun() + <-errCh + if status != 0 { + return cli.StatusError{StatusCode: status} } - - logrus.Debugf("Error hijack: %s", err) - return err } - status := <-statusChan - if status != 0 { - return cli.StatusError{StatusCode: status} - } return nil } diff --git a/e2e/container/run_test.go b/e2e/container/run_test.go index c835f1caf2..f4e591e21d 100644 --- a/e2e/container/run_test.go +++ b/e2e/container/run_test.go @@ -3,11 +3,13 @@ package container import ( "bytes" "fmt" + "os/exec" "strings" "syscall" "testing" "time" + "github.com/creack/pty" "github.com/docker/cli/e2e/internal/fixtures" "github.com/docker/cli/internal/test/environment" "github.com/docker/docker/api/types/versions" @@ -38,6 +40,39 @@ func TestRunAttachedFromRemoteImageAndRemove(t *testing.T) { golden.Assert(t, result.Stderr(), "run-attached-from-remote-and-remove.golden") } +func TestRunAttach(t *testing.T) { + skip.If(t, environment.RemoteDaemon()) + t.Parallel() + + streams := []string{"stdin", "stdout", "stderr"} + for _, stream := range streams { + t.Run(stream, func(t *testing.T) { + t.Parallel() + c := exec.Command("docker", "run", "-a", stream, "--rm", "alpine", + "sh", "-c", "sleep 1 && exit 7") + d := bytes.Buffer{} + c.Stdout = &d + c.Stderr = &d + _, err := pty.Start(c) + assert.NilError(t, err) + + done := make(chan error) + go func() { + done <- c.Wait() + }() + + select { + case <-time.After(20 * time.Second): + t.Fatal("docker run took too long, likely hang", d.String()) + case <-done: + } + + assert.Equal(t, c.ProcessState.ExitCode(), 7) + assert.Check(t, is.Contains(d.String(), "exit status 7")) + }) + } +} + // Regression test for https://github.com/docker/cli/issues/5053 func TestRunInvalidEntrypointWithAutoremove(t *testing.T) { environment.SkipIfDaemonNotLinux(t)