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’ll 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
Merged

windows: monitorTtySize correctly by polling #11843

merged 1 commit into from
Mar 31, 2015

Conversation

ahmetb
Copy link
Contributor

@ahmetb ahmetb 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

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
Copy link
Contributor Author

ahmetb 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
Copy link
Contributor

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

@ahmetb
Copy link
Contributor Author

ahmetb 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
Copy link
Contributor Author

ahmetb 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, “User 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
Copy link
Contributor

Make sense to me,

LGTM

@ahmetb
Copy link
Contributor Author

ahmetb commented Mar 30, 2015

ping: @tiborvass @icecrime

@crosbymichael
Copy link
Contributor

ping @jfrazelle @tiborvass

@jessfraz
Copy link
Contributor

LGTM

@tiborvass
Copy link
Contributor

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
windows: monitorTtySize correctly by polling
@tiborvass tiborvass merged commit ddbc68f into moby:master Mar 31, 2015
@ahmetb ahmetb deleted the win-cli/monitorttysize branch March 31, 2015 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants