Merge pull request #11406 from MabinGo/CmdStart_fix

Fix hanging up problem when start and attach multiple containers at once

api/client/commands.go

@@ -769,18 +769,6 @@ func (cli *DockerCli) CmdStart(args ...string) error {
 	cmd.Require(flag.Min, 1)
 	utils.ParseFlags(cmd, args, true)
 
-	hijacked := make(chan io.Closer)
-	// Block the return until the chan gets closed
-	defer func() {
-		log.Debugf("CmdStart() returned, defer waiting for hijack to finish.")
-		if _, ok := <-hijacked; ok {
-			log.Errorf("Hijack did not finish (chan still open)")
-		}
-		if *openStdin || *attach {
-			cli.in.Close()
-		}
-	}()
-
 	if *attach || *openStdin {
 		if cmd.NArg() > 1 {
 			return fmt.Errorf("You cannot start and attach multiple containers at once.")
@@ -816,12 +804,18 @@ func (cli *DockerCli) CmdStart(args ...string) error {
 		v.Set("stdout", "1")
 		v.Set("stderr", "1")
 
+		hijacked := make(chan io.Closer)
+		// Block the return until the chan gets closed
+		defer func() {
+			log.Debugf("CmdStart() returned, defer waiting for hijack to finish.")
+			if _, ok := <-hijacked; ok {
+				log.Errorf("Hijack did not finish (chan still open)")
+			}
+			cli.in.Close()
+		}()
 		cErr = promise.Go(func() error {
 			return cli.hijack("POST", "/containers/"+cmd.Arg(0)+"/attach?"+v.Encode(), tty, in, cli.out, cli.err, hijacked, nil)
 		})
-	} else {
-		close(hijacked)
-	}
 
 		// Acknowledge the hijack before starting
 		select {
@@ -836,6 +830,8 @@ func (cli *DockerCli) CmdStart(args ...string) error {
 				return err
 			}
 		}
+	}
+
 	var encounteredError error
 	for _, name := range cmd.Args() {
 		_, _, err := readBody(cli.call("POST", "/containers/"+name+"/start", nil, false))

integration-cli/docker_cli_start_test.go

@@ -240,3 +240,49 @@ func TestStartMultipleContainers(t *testing.T) {
 
 	logDone("start - start multiple containers continue on one failed")
 }
+
+func TestStartAttachMultipleContainers(t *testing.T) {
+
+	var cmd *exec.Cmd
+
+	defer deleteAllContainers()
+	// run  multiple containers to test
+	for _, container := range []string{"test1", "test2", "test3"} {
+		cmd = exec.Command(dockerBinary, "run", "-d", "--name", container, "busybox", "top")
+		if out, _, err := runCommandWithOutput(cmd); err != nil {
+			t.Fatal(out, err)
+		}
+	}
+
+	// stop all the containers
+	for _, container := range []string{"test1", "test2", "test3"} {
+		cmd = exec.Command(dockerBinary, "stop", container)
+		if out, _, err := runCommandWithOutput(cmd); err != nil {
+			t.Fatal(out, err)
+		}
+	}
+
+	// test start and attach multiple containers at once, expected error
+	for _, option := range []string{"-a", "-i", "-ai"} {
+		cmd = exec.Command(dockerBinary, "start", option, "test1", "test2", "test3")
+		out, _, err := runCommandWithOutput(cmd)
+		if !strings.Contains(out, "You cannot start and attach multiple containers at once.") || err == nil {
+			t.Fatal("Expected error but got none")
+		}
+	}
+
+	// confirm the state of all the containers be stopped
+	for container, expected := range map[string]string{"test1": "false", "test2": "false", "test3": "false"} {
+		cmd = exec.Command(dockerBinary, "inspect", "-f", "{{.State.Running}}", container)
+		out, _, err := runCommandWithOutput(cmd)
+		if err != nil {
+			t.Fatal(out, err)
+		}
+		out = strings.Trim(out, "\r\n")
+		if out != expected {
+			t.Fatal("Container running state wrong")
+		}
+	}
+
+	logDone("start - error on start and attach multiple containers at once")
+}