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.

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

Already on GitHub? Sign in to your account

windows: monitorTtySize correctly by polling #11843

Merged
merged 1 commit into from Mar 31, 2015

Conversation

Projects
None yet
7 participants
@ahmetb
Copy link
Contributor

commented Mar 26, 2015

This change makes monitorTtySize work correctly on windows by polling
into win32 API to get terminal size (because there's no SIGWINCH on
windows) and send it to the engine over Remove API properly.

Average getTtySize takes around 10-40 ms on an average windows machine
as far as I can tell (used go benchmarks), therefore in a for loop, checking
every 250ms if the size has changed since previous measurement or not.
If so, making the rest API call to update the term size.

That should be good enough to emulate SIGWINCH semantics on Windows.

I'm not sure if there's a better way to do it on windows, if so,
somebody please send a link 'cause I could not find.

Also not sure if this should go to 1.6 or not, just a UX improvement.

Demo:


(emphasis on sizes shown in the daemon logs) 馃槃

Signed-off-by: Ahmet Alp Balkan ahmetalpbalkan@gmail.com

windows: monitorTtySize correctly by polling
This change makes `monitorTtySize` work correctly on windows by polling
into win32 API to get terminal size (because there's no SIGWINCH on
windows) and send it to the engine over Remove API properly.

Average getttysize syscall takes around 30-40 ms on an average windows
machine as far as I can tell, therefore in a `for` loop, checking every
250ms if size has changed or not.

I'm not sure if there's a better way to do it on windows, if so,
somebody please send a link 'cause I could not find.

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
@ahmetb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2015

@jfrazelle Thanks! 鉂わ笍 I'm hoping to hear opinions on design, let's give it a day if we can.

@jessfraz jessfraz removed this from the 1.6.0 milestone Mar 26, 2015

@jessfraz

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2015

then let's not do 1.6 and we can see where we land :D

@ahmetb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2015

@jfrazelle this is safe enough to land in 1.6 and fixes a great deal of user experience on Windows. I don't see why not.

@jessfraz jessfraz added this to the 1.6.0 milestone Mar 26, 2015

@ahmetb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2015

OK I found some more info:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms686033(v=vs.85).aspx
In win32 API if we SetConsoleMode with ENABLE_WINDOW_INPUT bit, 鈥淯ser interactions that change the size of the console screen buffer are reported in the console's input buffer.鈥

So it is possible to do this without polling.

However, implementing that is probably beyond my ability at this time (I'm not very familiar with console input buffer is processed) and this proposed solution just works, too. Implementing ENABLE_INPUT_WINDOW would also require refactoring to invert the flow (i.e. currently the monitorTtySize is in api/client package and asks term package for events/size. If we refactor it would be platform specific and this method would go to terminal packages i.e. pkg/term & pkg/term/winconsole).

@crosbymichael

This comment has been minimized.

Copy link
Member

commented Mar 27, 2015

Make sense to me,

LGTM

@ahmetb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2015

@crosbymichael

This comment has been minimized.

Copy link
Member

commented Mar 30, 2015

@jessfraz

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2015

LGTM

@tiborvass

This comment has been minimized.

Copy link
Collaborator

commented Mar 31, 2015

LGTM

@ahmetalpbalkan I still see the panic unfortunately :( I believe it's as the buffer grows. After doing opening vim a couple of times, I made the window full screen and it panicked. But it's not related to the winresize itself, more of filling in buffers (and there are some dangling pointers in there that would need to be hunted somehow).

tiborvass added a commit that referenced this pull request Mar 31, 2015

Merge pull request #11843 from ahmetalpbalkan/win-cli/monitorttysize
windows: monitorTtySize correctly by polling

@tiborvass tiborvass merged commit ddbc68f into moby:master Mar 31, 2015

2 checks passed

janky Jenkins build Docker-PRs 4394 has succeeded
Details
windows Jenkins build Windows-PRs 1385 has succeeded
Details

@ahmetb ahmetb deleted the ahmetb:win-cli/monitorttysize branch Mar 31, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.