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

win, tty: fix problem of receiving unexpected SIGWINCH #2478

Closed
wants to merge 2 commits into from

Conversation

erw7
Copy link
Contributor

@erw7 erw7 commented Sep 16, 2019

Fix an issue where WINDOWS_BUFFER_SIZE_EVENT occurs and unexpected SIGWINCH is received before calling uv__tty_console_resize_message_loop_thread().

We encountered this problem in neovim functional tests.

Ref. neovim/neovim#10978 (comment)

Fix an issue where WINDOWS_BUFFER_SIZE_EVENT occurs and unexpected
SIGWINCH is received before calling
uv__tty_console_resize_message_loop_thread.
@erw7 erw7 force-pushed the fix-receiving-unexpected-sigwinch branch from d169b7c to de6613e Compare September 16, 2019 05:42
@erw7 erw7 mentioned this pull request Sep 16, 2019
@bnoordhuis
Copy link
Member

@bzoz You're probably the most qualified to review this.

@saghul
Copy link
Member

saghul commented Sep 16, 2019

Since this was caught with a test, any chance one can be written so we can catch it if it comes back in the future?

Copy link
Member

@bzoz bzoz left a comment

Choose a reason for hiding this comment

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

LGTM

@erw7
Copy link
Contributor Author

erw7 commented Sep 17, 2019

Since this was caught with a test, any chance one can be written so we can catch it if it comes back in the future?

To write a test that will catch this problem reliably, we need to call uv__tty_console_resize_message_loop_thread() after the first WINDOWS_BUFFER_SIZE_EVENT. I think it is possible to implement such behavior only during testing, but it will be too complicated.

1beb739 tried to add assert so that it can be caught when this problem recurs in the future. How about this?

@saghul
Copy link
Member

saghul commented Sep 17, 2019

That assert would trip if GetConsoleScreenBufferInfo had returned error, right? Is that possible? We might want to abort() if we are in a "the code is broken" situation, IMHO.

@erw7
Copy link
Contributor Author

erw7 commented Sep 18, 2019

That assert would trip if GetConsoleScreenBufferInfo had returned error, right? Is that possible? We might want to abort() if we are in a "the code is broken" situation, IMHO.

@saghul I'm not sure, but if the GetConsoleScreenBufferInfo() on line 177 returns an error, the assert will not fail because GetConsoleScreenBufferInfo() on line 2369 also returns an error.

@saghul
Copy link
Member

saghul commented Sep 18, 2019

Gotcha. LGTM.

saghul pushed a commit that referenced this pull request Sep 25, 2019
Fix an issue where WINDOWS_BUFFER_SIZE_EVENT occurs and unexpected
SIGWINCH is received before calling
uv__tty_console_resize_message_loop_thread.

Refs: neovim/neovim#10978 (comment)
PR-URL: #2478
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
@saghul
Copy link
Member

saghul commented Sep 25, 2019

Landed in 5013042, thank you!

@saghul saghul closed this Sep 25, 2019
@erw7 erw7 deleted the fix-receiving-unexpected-sigwinch branch October 17, 2019 05:18
musm pushed a commit to musm/libuv that referenced this pull request Jul 9, 2020
Fix an issue where WINDOWS_BUFFER_SIZE_EVENT occurs and unexpected
SIGWINCH is received before calling
uv__tty_console_resize_message_loop_thread.

Refs: neovim/neovim#10978 (comment)
PR-URL: libuv#2478
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
(cherry picked from commit 5013042)
musm pushed a commit to musm/libuv that referenced this pull request Jul 9, 2020
Fix an issue where WINDOWS_BUFFER_SIZE_EVENT occurs and unexpected
SIGWINCH is received before calling
uv__tty_console_resize_message_loop_thread.

Refs: neovim/neovim#10978 (comment)
PR-URL: libuv#2478
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Saúl Ibarra Corretgé <s@saghul.net>
(cherry picked from commit 5013042)
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.

None yet

4 participants