Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes 1492: tty initial size error #1529

Merged
merged 1 commit into from
Feb 12, 2019
Merged

fixes 1492: tty initial size error #1529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 25 additions & 14 deletions cli/command/container/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,24 @@ import (

type fakeClient struct {
client.Client
inspectFunc func(string) (types.ContainerJSON, error)
execInspectFunc func(execID string) (types.ContainerExecInspect, error)
execCreateFunc func(container string, config types.ExecConfig) (types.IDResponse, error)
createContainerFunc func(config *container.Config, hostConfig *container.HostConfig, networkingConfig *network.NetworkingConfig, containerName string) (container.ContainerCreateCreatedBody, error)
containerStartFunc func(container string, options types.ContainerStartOptions) error
imageCreateFunc func(parentReference string, options types.ImageCreateOptions) (io.ReadCloser, error)
infoFunc func() (types.Info, error)
containerStatPathFunc func(container, path string) (types.ContainerPathStat, error)
containerCopyFromFunc func(container, srcPath string) (io.ReadCloser, types.ContainerPathStat, error)
logFunc func(string, types.ContainerLogsOptions) (io.ReadCloser, error)
waitFunc func(string) (<-chan container.ContainerWaitOKBody, <-chan error)
containerListFunc func(types.ContainerListOptions) ([]types.Container, error)
containerExportFunc func(string) (io.ReadCloser, error)
Version string
inspectFunc func(string) (types.ContainerJSON, error)
execInspectFunc func(execID string) (types.ContainerExecInspect, error)
execCreateFunc func(container string, config types.ExecConfig) (types.IDResponse, error)
createContainerFunc func(config *container.Config,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this signature did not change; perhaps you can revert this formatting change to keep the change focussed on the actual changes? Or did gofmt force this wrap?

Copy link
Contributor Author

@lifubang lifubang Feb 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because in circleci:lint, there is an error:
cli/command/container/client_test.go:18::warning: line is 201 characters (lll),
so I need to force this wrap, or else ci will get an error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah! hm that's a pity (as it's not related to this change 😞)

hostConfig *container.HostConfig,
networkingConfig *network.NetworkingConfig,
containerName string) (container.ContainerCreateCreatedBody, error)
containerStartFunc func(container string, options types.ContainerStartOptions) error
imageCreateFunc func(parentReference string, options types.ImageCreateOptions) (io.ReadCloser, error)
infoFunc func() (types.Info, error)
containerStatPathFunc func(container, path string) (types.ContainerPathStat, error)
containerCopyFromFunc func(container, srcPath string) (io.ReadCloser, types.ContainerPathStat, error)
logFunc func(string, types.ContainerLogsOptions) (io.ReadCloser, error)
waitFunc func(string) (<-chan container.ContainerWaitOKBody, <-chan error)
containerListFunc func(types.ContainerListOptions) ([]types.Container, error)
containerExportFunc func(string) (io.ReadCloser, error)
containerExecResizeFunc func(id string, options types.ResizeOptions) error
Version string
}

func (f *fakeClient) ContainerList(_ context.Context, options types.ContainerListOptions) ([]types.Container, error) {
Expand Down Expand Up @@ -132,3 +136,10 @@ func (f *fakeClient) ContainerExport(_ context.Context, container string) (io.Re
}
return nil, nil
}

func (f *fakeClient) ContainerExecResize(_ context.Context, id string, options types.ResizeOptions) error {
if f.containerExecResizeFunc != nil {
return f.containerExecResizeFunc(id, options)
}
return nil
}
47 changes: 35 additions & 12 deletions cli/command/container/tty.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ import (
)

// resizeTtyTo resizes tty to specific height and width
func resizeTtyTo(ctx context.Context, client client.ContainerAPIClient, id string, height, width uint, isExec bool) {
func resizeTtyTo(ctx context.Context, client client.ContainerAPIClient, id string, height, width uint, isExec bool) error {
if height == 0 && width == 0 {
return
return nil
}

options := types.ResizeOptions{
Expand All @@ -34,19 +34,42 @@ func resizeTtyTo(ctx context.Context, client client.ContainerAPIClient, id strin
}

if err != nil {
logrus.Debugf("Error resize: %s", err)
logrus.Debugf("Error resize: %s\r", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for adding \r here?

Copy link
Contributor Author

@lifubang lifubang Nov 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kolyshkin Nice to see you again ^_^
Thanks for your review. It's because that:

root@iZ2zeesy3n96esegm7tue0Z:~# docker --debug exec -it k8s_nginxgateway_apigateway-p46ch_default_2de895e0-e8a8-11e8-badf-00163e0af11c_0 /bin/bash
DEBU[0000] Error resize: Error response from daemon: no such exec 
                                                                  root@apigateway-p46ch:/# ls
bin   dev  home  lib64  mnt  proc  run   srv  t    usr
boot  etc  lib   media  opt  root  sbin  sys  tmp  var
root@apigateway-p46ch:/#

When we use --debug, the first shell prompt don't return to the head of line!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! I've definitely seen it in the past.

Maybe add a comment why \r is there to the patch description, so anyone doing git blame can find the reason why.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No code change, modify patch description now. Thanks.

}
return err
}

// MonitorTtySize updates the container tty size when the terminal tty changes size
func MonitorTtySize(ctx context.Context, cli command.Cli, id string, isExec bool) error {
resizeTty := func() {
height, width := cli.Out().GetTtySize()
resizeTtyTo(ctx, cli.Client(), id, height, width, isExec)
}
// resizeTty is to resize the tty with cli out's tty size
func resizeTty(ctx context.Context, cli command.Cli, id string, isExec bool) error {
height, width := cli.Out().GetTtySize()
return resizeTtyTo(ctx, cli.Client(), id, height, width, isExec)
}

resizeTty()
// initTtySize is to init the tty's size to the same as the window, if there is an error, it will retry 5 times.
func initTtySize(ctx context.Context, cli command.Cli, id string, isExec bool, resizeTtyFunc func(ctx context.Context, cli command.Cli, id string, isExec bool) error) {
rttyFunc := resizeTtyFunc
if rttyFunc == nil {
rttyFunc = resizeTty
}
if err := rttyFunc(ctx, cli, id, isExec); err != nil {
go func() {
var err error
for retry := 0; retry < 5; retry++ {
time.Sleep(10 * time.Millisecond)
if err = rttyFunc(ctx, cli, id, isExec); err == nil {
break
}
}
if err != nil {
fmt.Fprintln(cli.Err(), "failed to resize tty, using default size")
}
}()
}
}

// MonitorTtySize updates the container tty size when the terminal tty changes size
func MonitorTtySize(ctx context.Context, cli command.Cli, id string, isExec bool) error {
initTtySize(ctx, cli, id, isExec, resizeTty)
if runtime.GOOS == "windows" {
go func() {
prevH, prevW := cli.Out().GetTtySize()
Expand All @@ -55,7 +78,7 @@ func MonitorTtySize(ctx context.Context, cli command.Cli, id string, isExec bool
h, w := cli.Out().GetTtySize()

if prevW != w || prevH != h {
resizeTty()
resizeTty(ctx, cli, id, isExec)
}
prevH = h
prevW = w
Expand All @@ -66,7 +89,7 @@ func MonitorTtySize(ctx context.Context, cli command.Cli, id string, isExec bool
gosignal.Notify(sigchan, signal.SIGWINCH)
go func() {
for range sigchan {
resizeTty()
resizeTty(ctx, cli, id, isExec)
}
}()
}
Expand Down
30 changes: 30 additions & 0 deletions cli/command/container/tty_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package container

import (
"context"
"testing"
"time"

"github.com/docker/cli/cli/command"
"github.com/docker/cli/internal/test"
"github.com/docker/docker/api/types"
"github.com/pkg/errors"
"gotest.tools/assert"
is "gotest.tools/assert/cmp"
)

func TestInitTtySizeErrors(t *testing.T) {
expectedError := "failed to resize tty, using default size\n"
fakeContainerExecResizeFunc := func(id string, options types.ResizeOptions) error {
return errors.Errorf("Error response from daemon: no such exec")
}
fakeResizeTtyFunc := func(ctx context.Context, cli command.Cli, id string, isExec bool) error {
height, width := uint(1024), uint(768)
return resizeTtyTo(ctx, cli.Client(), id, height, width, isExec)
}
ctx := context.Background()
cli := test.NewFakeCli(&fakeClient{containerExecResizeFunc: fakeContainerExecResizeFunc})
initTtySize(ctx, cli, "8mm8nn8tt8bb", true, fakeResizeTtyFunc)
time.Sleep(100 * time.Millisecond)
assert.Check(t, is.Equal(expectedError, cli.ErrBuffer().String()))
}