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 <laurabrehm@hey.com>

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
 }
 

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)