Merge pull request #5662 from laurazard/consistent-attach-check

run: correctly handle only STDIN attached

Author: thaJeztah

cli/command/container/run.go

@@ -161,15 +161,15 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption
 		waitDisplayID chan struct{}
 		errCh         chan error
 	)
-	if !config.AttachStdout && !config.AttachStderr {
+	attach := config.AttachStdin || config.AttachStdout || config.AttachStderr
+	if !attach {
 		// Make this asynchronous to allow the client to write to stdin before having to read the ID
 		waitDisplayID = make(chan struct{})
 		go func() {
 			defer close(waitDisplayID)
 			_, _ = fmt.Fprintln(stdout, containerID)
 		}()
 	}
-	attach := config.AttachStdin || config.AttachStdout || config.AttachStderr
 	if attach {
 		detachKeys := dockerCli.ConfigFile().DetachKeys
 		if runOpts.detachKeys != "" {
@@ -215,14 +215,22 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption
 		return toStatusError(err)
 	}
 
-	if (config.AttachStdin || config.AttachStdout || config.AttachStderr) && config.Tty && dockerCli.Out().IsTerminal() {
+	// Detached mode: wait for the id to be displayed and return.
+	if !attach {
+		// Detached mode
+		<-waitDisplayID
+		return nil
+	}
+
+	if config.Tty && dockerCli.Out().IsTerminal() {
 		if err := MonitorTtySize(ctx, dockerCli, containerID, false); err != nil {
 			_, _ = fmt.Fprintln(stderr, "Error monitoring TTY size:", err)
 		}
 	}
 
-	if errCh != nil {
-		if err := <-errCh; err != nil {
+	select {
+	case err := <-errCh:
+		if err != nil {
 			if _, ok := err.(term.EscapeError); ok {
 				// The user entered the detach escape sequence.
 				return nil
@@ -231,19 +239,20 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption
 			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}
+		}
 	}
 
-	// Detached mode: wait for the id to be displayed and return.
-	if !config.AttachStdout && !config.AttachStderr {
-		// Detached mode
-		<-waitDisplayID
-		return nil
-	}
-
-	status := <-statusChan
-	if status != 0 {
-		return cli.StatusError{StatusCode: status}
-	}
 	return nil
 }
 

e2e/container/attach_test.go

@@ -37,7 +37,6 @@ func TestAttachInterrupt(t *testing.T) {
 	// todo(laurazard): make this test work w/ dind over ssh
 	skip.If(t, strings.Contains(os.Getenv("DOCKER_HOST"), "ssh://"))
 
-	// if
 	result := icmd.RunCommand("docker", "run", "-d", fixtures.AlpineImage,
 		"sh", "-c", "trap \"exit 33\" SIGINT; for i in $(seq 100); do sleep 0.1; done; exit 34")
 	result.Assert(t, icmd.Success)

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)