run: don't hang if only attaching STDIN

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 <[email protected]>
docker · Dec 2, 2024 · a8650d8 · a8650d8
1 parent a3d9fc4
commit a8650d8
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 11 deletions.
diff --git 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
+			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}
+		}
 	}
 
-	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
@@ -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,37 @@ 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())
+
+	streams := []string{"stdin", "stdout", "stderr"}
+	for _, stream := range streams {
+		t.Run(stream, func(t *testing.T) {
+			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, strings.Contains(d.String(), "exit status 7"))
+		})
+	}
+}
+
 // Regression test for https://github.com/docker/cli/issues/5053
 func TestRunInvalidEntrypointWithAutoremove(t *testing.T) {
 	environment.SkipIfDaemonNotLinux(t)