From 0d916a930b6c01d9bd60586f93bc4194b3833262 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Thu, 4 Apr 2013 11:23:51 -0700 Subject: [PATCH 1/6] Merge the 3 ptys in 1 Upstream-commit: 847a8f45a45dccd0574396b888670004e32762e6 Component: engine --- components/engine/container.go | 31 ++++--------------------------- 1 file changed, 4 insertions(+), 27 deletions(-) diff --git a/components/engine/container.go b/components/engine/container.go index 76e9f5a2d7..2fa46ada2e 100644 --- a/components/engine/container.go +++ b/components/engine/container.go @@ -180,19 +180,14 @@ func (container *Container) generateLXCConfig() error { } func (container *Container) startPty() error { + stdoutMaster, stdoutSlave, err := pty.Open() if err != nil { return err } container.ptyStdoutMaster = stdoutMaster container.cmd.Stdout = stdoutSlave - - stderrMaster, stderrSlave, err := pty.Open() - if err != nil { - return err - } - container.ptyStderrMaster = stderrMaster - container.cmd.Stderr = stderrSlave + container.cmd.Stderr = stdoutSlave // Copy the PTYs to our broadcasters go func() { @@ -202,30 +197,16 @@ func (container *Container) startPty() error { Debugf("[startPty] End of stdout pipe") }() - go func() { - defer container.stderr.CloseWriters() - Debugf("[startPty] Begin of stderr pipe") - io.Copy(container.stderr, stderrMaster) - Debugf("[startPty] End of stderr pipe") - }() - // stdin - var stdinSlave io.ReadCloser if container.Config.OpenStdin { - var stdinMaster io.WriteCloser - stdinMaster, stdinSlave, err = pty.Open() - if err != nil { - return err - } - container.ptyStdinMaster = stdinMaster - container.cmd.Stdin = stdinSlave + container.cmd.Stdin = stdoutSlave // FIXME: The following appears to be broken. // "cannot set terminal process group (-1): Inappropriate ioctl for device" // container.cmd.SysProcAttr = &syscall.SysProcAttr{Setctty: true, Setsid: true} go func() { defer container.stdin.Close() Debugf("[startPty] Begin of stdin pipe") - io.Copy(stdinMaster, container.stdin) + io.Copy(stdoutMaster, container.stdin) Debugf("[startPty] End of stdin pipe") }() } @@ -233,10 +214,6 @@ func (container *Container) startPty() error { return err } stdoutSlave.Close() - stderrSlave.Close() - if stdinSlave != nil { - stdinSlave.Close() - } return nil } From 8f7ae9e9559bce955bbcb1d27566f6b03f4426f4 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Thu, 4 Apr 2013 11:24:22 -0700 Subject: [PATCH 2/6] Make sure the process start in his own session and grabs the terminal Upstream-commit: 33a5fe3bd4cb0bd31b01da89c8c4c3321701e7a1 Component: engine --- components/engine/container.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/engine/container.go b/components/engine/container.go index 2fa46ada2e..2d20ce510c 100644 --- a/components/engine/container.go +++ b/components/engine/container.go @@ -202,7 +202,7 @@ func (container *Container) startPty() error { container.cmd.Stdin = stdoutSlave // FIXME: The following appears to be broken. // "cannot set terminal process group (-1): Inappropriate ioctl for device" - // container.cmd.SysProcAttr = &syscall.SysProcAttr{Setctty: true, Setsid: true} + container.cmd.SysProcAttr = &syscall.SysProcAttr{Setctty: true, Setsid: true} go func() { defer container.stdin.Close() Debugf("[startPty] Begin of stdin pipe") From 561e5a7afb9e1ca946c8d1fcb1f792e32ecdea72 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Thu, 4 Apr 2013 12:12:22 -0700 Subject: [PATCH 3/6] Cleanup pty variable names Upstream-commit: 7d8895545e2442ea095f83c90edff64835a0d0b5 Component: engine --- components/engine/container.go | 39 +++++++++++----------------------- 1 file changed, 12 insertions(+), 27 deletions(-) diff --git a/components/engine/container.go b/components/engine/container.go index 2d20ce510c..aca82d4cbe 100644 --- a/components/engine/container.go +++ b/components/engine/container.go @@ -40,9 +40,7 @@ type Container struct { stdin io.ReadCloser stdinPipe io.WriteCloser - ptyStdinMaster io.Closer - ptyStdoutMaster io.Closer - ptyStderrMaster io.Closer + ptyMaster io.Closer runtime *Runtime } @@ -180,40 +178,37 @@ func (container *Container) generateLXCConfig() error { } func (container *Container) startPty() error { - - stdoutMaster, stdoutSlave, err := pty.Open() + ptyMaster, ptySlave, err := pty.Open() if err != nil { return err } - container.ptyStdoutMaster = stdoutMaster - container.cmd.Stdout = stdoutSlave - container.cmd.Stderr = stdoutSlave + container.ptyMaster = ptyMaster + container.cmd.Stdout = ptySlave + container.cmd.Stderr = ptySlave // Copy the PTYs to our broadcasters go func() { defer container.stdout.CloseWriters() Debugf("[startPty] Begin of stdout pipe") - io.Copy(container.stdout, stdoutMaster) + io.Copy(container.stdout, ptyMaster) Debugf("[startPty] End of stdout pipe") }() // stdin if container.Config.OpenStdin { - container.cmd.Stdin = stdoutSlave - // FIXME: The following appears to be broken. - // "cannot set terminal process group (-1): Inappropriate ioctl for device" + container.cmd.Stdin = ptySlave container.cmd.SysProcAttr = &syscall.SysProcAttr{Setctty: true, Setsid: true} go func() { defer container.stdin.Close() Debugf("[startPty] Begin of stdin pipe") - io.Copy(stdoutMaster, container.stdin) + io.Copy(ptyMaster, container.stdin) Debugf("[startPty] End of stdin pipe") }() } if err := container.cmd.Start(); err != nil { return err } - stdoutSlave.Close() + ptySlave.Close() return nil } @@ -507,19 +502,9 @@ func (container *Container) monitor() { Debugf("%s: Error close stderr: %s", container.Id, err) } - if container.ptyStdinMaster != nil { - if err := container.ptyStdinMaster.Close(); err != nil { - Debugf("%s: Error close pty stdin master: %s", container.Id, err) - } - } - if container.ptyStdoutMaster != nil { - if err := container.ptyStdoutMaster.Close(); err != nil { - Debugf("%s: Error close pty stdout master: %s", container.Id, err) - } - } - if container.ptyStderrMaster != nil { - if err := container.ptyStderrMaster.Close(); err != nil { - Debugf("%s: Error close pty stderr master: %s", container.Id, err) + if container.ptyMaster != nil { + if err := container.ptyMaster.Close(); err != nil { + Debugf("%s: Error closing Pty master: %s", container.Id, err) } } From 91c3817cddffc62aed5f5d696712cdee428c3390 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Fri, 5 Apr 2013 19:02:35 -0700 Subject: [PATCH 4/6] Fix run disconnect behavious in tty mode + add unit test to enforce it Upstream-commit: 99b5bec0692ea9ec8a397926df9dd545fb264ac9 Component: engine --- components/engine/commands_test.go | 48 ++++++++++++++++++++++++++++++ components/engine/container.go | 3 ++ 2 files changed, 51 insertions(+) diff --git a/components/engine/commands_test.go b/components/engine/commands_test.go index 6c9dc70d5e..4592ea77ac 100644 --- a/components/engine/commands_test.go +++ b/components/engine/commands_test.go @@ -191,6 +191,54 @@ func TestRunDisconnect(t *testing.T) { }) } +// Expected behaviour: the process dies when the client disconnects +func TestRunDisconnectTty(t *testing.T) { + runtime, err := newTestRuntime() + if err != nil { + t.Fatal(err) + } + defer nuke(runtime) + + srv := &Server{runtime: runtime} + + stdin, stdinPipe := io.Pipe() + stdout, stdoutPipe := io.Pipe() + c1 := make(chan struct{}) + go func() { + // We're simulating a disconnect so the return value doesn't matter. What matters is the + // fact that CmdRun returns. + srv.CmdRun(stdin, rcli.NewDockerLocalConn(stdoutPipe), "-i", "-t", GetTestImage(runtime).Id, "/bin/cat") + close(c1) + }() + + setTimeout(t, "Read/Write assertion timed out", 2*time.Second, func() { + if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 15); err != nil { + t.Fatal(err) + } + }) + + // Close pipes (simulate disconnect) + if err := closeWrap(stdin, stdinPipe, stdout, stdoutPipe); err != nil { + t.Fatal(err) + } + + // as the pipes are close, we expect the process to die, + // therefore CmdRun to unblock. Wait for CmdRun + setTimeout(t, "Waiting for CmdRun timed out", 2*time.Second, func() { + <-c1 + }) + + // Client disconnect after run -i should cause stdin to be closed, which should + // cause /bin/cat to exit. + setTimeout(t, "Waiting for /bin/cat to exit timed out", 2*time.Second, func() { + container := runtime.List()[0] + container.Wait() + if container.State.Running { + t.Fatalf("/bin/cat is still running after closing stdin") + } + }) +} + // TestAttachStdin checks attaching to stdin without stdout and stderr. // 'docker run -i -a stdin' should sends the client's stdin to the command, // then detach from it and print the container id. diff --git a/components/engine/container.go b/components/engine/container.go index aca82d4cbe..6b3913522c 100644 --- a/components/engine/container.go +++ b/components/engine/container.go @@ -251,6 +251,9 @@ func (container *Container) Attach(stdin io.ReadCloser, stdinCloser io.Closer, s defer cStderr.Close() } if container.Config.StdinOnce { + if container.Config.Tty { + defer container.Kill() + } defer cStdin.Close() } _, err := io.Copy(cStdin, stdin) From b2cdbb0f9857cdd6d151ea0af7019538b3c9f1f1 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Fri, 5 Apr 2013 19:48:49 -0700 Subject: [PATCH 5/6] Make sure to flush buffer when setting raw mode Upstream-commit: 7e1e7d14fa257692231bc7e13e796e07607879c2 Component: engine --- components/engine/commands.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/components/engine/commands.go b/components/engine/commands.go index 7e62596570..25889b46f9 100644 --- a/components/engine/commands.go +++ b/components/engine/commands.go @@ -803,6 +803,8 @@ func (srv *Server) CmdAttach(stdin io.ReadCloser, stdout rcli.DockerConn, args . if container.Config.Tty { stdout.SetOptionRawTerminal() + // Flush the options to make sure the client sets the raw mode + stdout.Write([]byte{}) } return <-container.Attach(stdin, nil, stdout, stdout) } @@ -888,8 +890,11 @@ func (srv *Server) CmdRun(stdin io.ReadCloser, stdout rcli.DockerConn, args ...s fmt.Fprintln(stdout, "Error: Command not specified") return fmt.Errorf("Command not specified") } + if config.Tty { stdout.SetOptionRawTerminal() + // Flush the options to make sure the client sets the raw mode + stdout.Write([]byte{}) } // Create new container From c8a8294b68458a15c8f41a50432c275e076060ad Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Fri, 5 Apr 2013 20:08:31 -0700 Subject: [PATCH 6/6] Move the DockerConn flush to its own function Upstream-commit: c83393a541353ab34612d80cc6b9bb4e92c59597 Component: engine --- components/engine/commands.go | 2 +- components/engine/rcli/tcp.go | 5 +++++ components/engine/rcli/types.go | 3 +++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/components/engine/commands.go b/components/engine/commands.go index 25889b46f9..29508ce0a3 100644 --- a/components/engine/commands.go +++ b/components/engine/commands.go @@ -894,7 +894,7 @@ func (srv *Server) CmdRun(stdin io.ReadCloser, stdout rcli.DockerConn, args ...s if config.Tty { stdout.SetOptionRawTerminal() // Flush the options to make sure the client sets the raw mode - stdout.Write([]byte{}) + stdout.Flush() } // Create new container diff --git a/components/engine/rcli/tcp.go b/components/engine/rcli/tcp.go index 6fbf2abd09..8c990ed82f 100644 --- a/components/engine/rcli/tcp.go +++ b/components/engine/rcli/tcp.go @@ -92,6 +92,11 @@ func (c *DockerTCPConn) Write(b []byte) (int, error) { return n + optionsLen, err } +func (c *DockerTCPConn) Flush() error { + _, err := c.conn.Write([]byte{}) + return err +} + func (c *DockerTCPConn) Close() error { return c.conn.Close() } func (c *DockerTCPConn) CloseWrite() error { return c.conn.CloseWrite() } diff --git a/components/engine/rcli/types.go b/components/engine/rcli/types.go index 791736a79c..38f4a8c008 100644 --- a/components/engine/rcli/types.go +++ b/components/engine/rcli/types.go @@ -29,6 +29,7 @@ type DockerConn interface { CloseRead() error GetOptions() *DockerConnOptions SetOptionRawTerminal() + Flush() error } type DockerLocalConn struct { @@ -56,6 +57,8 @@ func (c *DockerLocalConn) Close() error { return c.writer.Close() } +func (c *DockerLocalConn) Flush() error { return nil } + func (c *DockerLocalConn) CloseWrite() error { return nil } func (c *DockerLocalConn) CloseRead() error { return nil }