From 66d3dcc6f7648ab5af3c7f876c2b9c63ab2899ac Mon Sep 17 00:00:00 2001 From: Lei Jitang Date: Thu, 24 Mar 2016 21:25:50 -0400 Subject: [PATCH] cli: move setRawTerminal and restoreTerminal to holdHijackedConnection In this way, we can restore the Terminal as soon as possible once the hijacked connection end. This not only fix weird output if cli enable -D, but also remove duplicate code. Signed-off-by: Lei Jitang --- api/client/attach.go | 9 +-------- api/client/exec.go | 8 +------- api/client/hijack.go | 47 +++++++++++++++++++++++++++++++++++++++----- api/client/run.go | 23 +++++++++++++--------- api/client/start.go | 13 +++++------- 5 files changed, 63 insertions(+), 37 deletions(-) diff --git a/api/client/attach.go b/api/client/attach.go index e89644d6b..144d47efb 100644 --- a/api/client/attach.go +++ b/api/client/attach.go @@ -71,12 +71,6 @@ func (cli *DockerCli) CmdAttach(args ...string) error { return err } defer resp.Close() - if in != nil && c.Config.Tty { - if err := cli.setRawTerminal(); err != nil { - return err - } - defer cli.restoreTerminal(in) - } if c.Config.Tty && cli.isTerminalOut { height, width := cli.getTtySize() @@ -92,8 +86,7 @@ func (cli *DockerCli) CmdAttach(args ...string) error { logrus.Debugf("Error monitoring TTY size: %s", err) } } - - if err := cli.holdHijackedConnection(c.Config.Tty, in, cli.out, cli.err, resp); err != nil { + if err := cli.holdHijackedConnection(context.Background(), c.Config.Tty, in, cli.out, cli.err, resp); err != nil { return err } diff --git a/api/client/exec.go b/api/client/exec.go index 520c3a382..c96f03a0b 100644 --- a/api/client/exec.go +++ b/api/client/exec.go @@ -89,14 +89,8 @@ func (cli *DockerCli) CmdExec(args ...string) error { return err } defer resp.Close() - if in != nil && execConfig.Tty { - if err := cli.setRawTerminal(); err != nil { - return err - } - defer cli.restoreTerminal(in) - } errCh = promise.Go(func() error { - return cli.holdHijackedConnection(execConfig.Tty, in, out, stderr, resp) + return cli.holdHijackedConnection(context.Background(), execConfig.Tty, in, out, stderr, resp) }) if execConfig.Tty && cli.isTerminalIn { diff --git a/api/client/hijack.go b/api/client/hijack.go index 4c80fe1cd..38bd6a7af 100644 --- a/api/client/hijack.go +++ b/api/client/hijack.go @@ -2,23 +2,48 @@ package client import ( "io" + "sync" + + "golang.org/x/net/context" "github.com/Sirupsen/logrus" "github.com/docker/docker/pkg/stdcopy" "github.com/docker/engine-api/types" ) -func (cli *DockerCli) holdHijackedConnection(tty bool, inputStream io.ReadCloser, outputStream, errorStream io.Writer, resp types.HijackedResponse) error { - var err error +func (cli *DockerCli) holdHijackedConnection(ctx context.Context, tty bool, inputStream io.ReadCloser, outputStream, errorStream io.Writer, resp types.HijackedResponse) error { + var ( + err error + restoreOnce sync.Once + ) + if inputStream != nil && tty { + if err := cli.setRawTerminal(); err != nil { + return err + } + defer func() { + restoreOnce.Do(func() { + cli.restoreTerminal(inputStream) + }) + }() + } + receiveStdout := make(chan error, 1) if outputStream != nil || errorStream != nil { go func() { // When TTY is ON, use regular copy if tty && outputStream != nil { _, err = io.Copy(outputStream, resp.Reader) + // we should restore the terminal as soon as possible once connection end + // so any following print messages will be in normal type. + if inputStream != nil { + restoreOnce.Do(func() { + cli.restoreTerminal(inputStream) + }) + } } else { _, err = stdcopy.StdCopy(outputStream, errorStream, resp.Reader) } + logrus.Debugf("[hijack] End of stdout") receiveStdout <- err }() @@ -28,6 +53,13 @@ func (cli *DockerCli) holdHijackedConnection(tty bool, inputStream io.ReadCloser go func() { if inputStream != nil { io.Copy(resp.Conn, inputStream) + // we should restore the terminal as soon as possible once connection end + // so any following print messages will be in normal type. + if tty { + restoreOnce.Do(func() { + cli.restoreTerminal(inputStream) + }) + } logrus.Debugf("[hijack] End of stdin") } @@ -45,11 +77,16 @@ func (cli *DockerCli) holdHijackedConnection(tty bool, inputStream io.ReadCloser } case <-stdinDone: if outputStream != nil || errorStream != nil { - if err := <-receiveStdout; err != nil { - logrus.Debugf("Error receiveStdout: %s", err) - return err + select { + case err := <-receiveStdout: + if err != nil { + logrus.Debugf("Error receiveStdout: %s", err) + return err + } + case <-ctx.Done(): } } + case <-ctx.Done(): } return nil diff --git a/api/client/run.go b/api/client/run.go index 2e8323104..36155a31e 100644 --- a/api/client/run.go +++ b/api/client/run.go @@ -159,6 +159,8 @@ func (cli *DockerCli) CmdRun(args ...string) error { var ( waitDisplayID chan struct{} errCh chan error + cancelFun context.CancelFunc + ctx context.Context ) if !config.AttachStdout && !config.AttachStderr { // Make this asynchronous to allow the client to write to stdin before having to read the ID @@ -171,8 +173,8 @@ func (cli *DockerCli) CmdRun(args ...string) error { if *flAutoRemove && (hostConfig.RestartPolicy.IsAlways() || hostConfig.RestartPolicy.IsOnFailure()) { return ErrConflictRestartPolicyAndAutoRemove } - - if config.AttachStdin || config.AttachStdout || config.AttachStderr { + attach := config.AttachStdin || config.AttachStdout || config.AttachStderr + if attach { var ( out, stderr io.Writer in io.ReadCloser @@ -208,14 +210,9 @@ func (cli *DockerCli) CmdRun(args ...string) error { if err != nil { return err } - if in != nil && config.Tty { - if err := cli.setRawTerminal(); err != nil { - return err - } - defer cli.restoreTerminal(in) - } + ctx, cancelFun = context.WithCancel(context.Background()) errCh = promise.Go(func() error { - return cli.holdHijackedConnection(config.Tty, in, out, stderr, resp) + return cli.holdHijackedConnection(ctx, config.Tty, in, out, stderr, resp) }) } @@ -229,6 +226,14 @@ func (cli *DockerCli) CmdRun(args ...string) error { //start the container if err := cli.client.ContainerStart(context.Background(), createResponse.ID); err != nil { + // If we have holdHijackedConnection, we should notify + // holdHijackedConnection we are going to exit and wait + // to avoid the terminal are not restored. + if attach { + cancelFun() + <-errCh + } + cmd.ReportError(err.Error(), false) return runStartContainerErr(err) } diff --git a/api/client/start.go b/api/client/start.go index 1ff2845f5..0d141da88 100644 --- a/api/client/start.go +++ b/api/client/start.go @@ -89,6 +89,7 @@ func (cli *DockerCli) CmdStart(args ...string) error { } var in io.ReadCloser + if options.Stdin { in = cli.in } @@ -98,19 +99,15 @@ func (cli *DockerCli) CmdStart(args ...string) error { return err } defer resp.Close() - if in != nil && c.Config.Tty { - if err := cli.setRawTerminal(); err != nil { - return err - } - defer cli.restoreTerminal(in) - } - + ctx, cancelFun := context.WithCancel(context.Background()) cErr := promise.Go(func() error { - return cli.holdHijackedConnection(c.Config.Tty, in, cli.out, cli.err, resp) + return cli.holdHijackedConnection(ctx, c.Config.Tty, in, cli.out, cli.err, resp) }) // 3. Start the container. if err := cli.client.ContainerStart(context.Background(), containerID); err != nil { + cancelFun() + <-cErr return err }