-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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 #1380
Conversation
cc0e7c0
to
2db6708
Compare
Codecov Report
@@ Coverage Diff @@
## master #1380 +/- ##
==========================================
- Coverage 55.23% 54.91% -0.33%
==========================================
Files 289 292 +3
Lines 19374 19287 -87
==========================================
- Hits 10702 10591 -111
- Misses 7977 8034 +57
+ Partials 695 662 -33 |
2db6708
to
00f8eb8
Compare
LGTM aside from a small nitpick |
@lifubang can you please merge your commits? It's easier to review that way (in case the second commit fixes the first one etc.) |
@kolyshkin I have find the exact error when I met this situation. So we need to deal with this error. |
b238eb9
to
92dcaf7
Compare
@kolyshkin I have update the commit message: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM, but added this to today's maintainers meeting to have a quick check with other maintainers.
thanks!
cli/command/container/tty.go
Outdated
resizeTty() | ||
err := resizeTty() | ||
if err != nil { | ||
go func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussing in the maintainers meeting with @cpuguy83 and @kolyshkin - and we're wondering if we should use a retry-loop (with a maximum number of retries) instead.
This retry will make it less racy, but it's still possible that if fails; a loop would probably prevent that situation
@lifubang wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if there is a retry-loop (with a maximum number of retries), I think it will be better.
How about the result of the maintainers' discussion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you update the PR to use a retry loop, it should be ready to go, and we can merge.
Not sure if we can write a test for this; I think that would be complicated, so I'm ok with merging without a test, but if you can think of a way to test this, that would be great (but again; that's not a blocker for me)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's very hard to add a test case, because there are no return value in this func, so I let a debug info here. I'll think about it.
92dcaf7
to
63a773b
Compare
cli/command/container/tty.go
Outdated
err = resizeTty() | ||
if err != nil && retry < retryTimes { | ||
retry++ | ||
logrus.Debugf("Resize tty has retried %d times now\r", retry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be a bit noisy; also, since this is the CLI, I think this will be printed on stdout
/ stderr
, not logged (I think).
I think we should either remove the debug, or only print a debug message if it failed to do the resize after retryTimes
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I also add a small test case.
06fd12f
to
6bd526c
Compare
6bd526c
to
422baf6
Compare
Signed-off-by: Lifubang lifubang@acmcoder.com
fixes #1492.
For Ubuntu 16.04.2 LTS (GNU/Linux 4.4.0-62-generic x86_64), Docker version 17.12.0-ce, build c97c6d6 / 18.03.0-ce, build 0520e24
When I run:
docker exec -it ubuntu /bin/bash
The tty's size is small than window's size.
After I fix the bug:
If not fix this bug, it's too inconvenient for me to use docker exec -it.
- What I did
Fix tty initial size error
I think it's a bug of dockerd, but for previous dockerd edition, I think we can fix it in cli.
- How I did it
func MonitorTtySize in file cli/command/container/tty.goI move the call resizeTty() to last and run it asynchronously.Because in sometimes when dockercli call tty resize, the exec have not finished yet. Then we got a message "Error response from daemon: no such exec", but we ignore it, so we need to throw this error and deal with it after 10 milliseconds.
- How to verify it
It's convenient for me to use docker exec -it.
I don't need to resize the window.
- Description for the changelog
fixes tty initial size error
- I have no more cute animal now
Before fix:
vim /etc/mime.types
After fix:
vim /etc/mime.types