win,tty: improve SIGWINCH support #1408

bzoz · 2017-07-07T15:43:18Z

Add SetWinEventHook for EVENT_CONSOLE_LAYOUT for better detection of
console resize events.

bzoz · 2017-07-07T15:44:54Z

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/318/

saghul · 2017-07-24T22:37:47Z

src/win/tty.c

+    /* Store the global tty output handle. This handle is used by TTY read */
+    /* streams to update the virtual window when a CONSOLE_BUFFER_SIZE_EVENT */
+    /* is received. */
+    uv_tty_console_handle = handle;


we need to hold the uv_tty_output_lock at least here, this is not thread safe.

bzoz · 2017-07-28T10:37:23Z

@saghul updated (and rebased), PTAL

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/354/

saghul · 2017-07-29T14:11:33Z

src/win/tty.c

+ * handle signalling SIGWINCH
+ */
+
+static HANDLE uv_tty_console_handle = INVALID_HANDLE_VALUE;


can you prefix these with uv__ instead? I know src/win is not very consistent in this regard, but let's try to be, for new code.

saghul · 2017-07-29T14:12:59Z

src/win/tty.c

+                       WINEVENT_OUTOFCONTEXT))
+    uv_fatal_error(GetLastError(), "SetWinEventHook");
+
+  while (GetMessage(&msg, NULL, 0, 0)) {


how is this intettupted?

I've described it in #1408 (comment)

saghul · 2017-07-29T14:14:21Z

src/win/tty.c

+  height = screen_buffer_info.srWindow.Bottom - screen_buffer_info.srWindow.Top + 1;
+
+  if (width != uv_tty_console_width || height != uv_tty_console_height) {
+    uv_tty_console_width = width;


this needs to hold the lock

saghul · 2017-07-29T14:14:37Z

src/win/tty.c

+  if (!GetConsoleScreenBufferInfo(uv_tty_console_handle, &screen_buffer_info))
+    uv_fatal_error(GetLastError(), "GetConsoleScreenBufferInfo");
+
+  uv_tty_console_width = screen_buffer_info.dwSize.X;


this needs to hold the lock

bzoz · 2017-08-07T12:19:02Z

@saghul I've updated the change, PTAL.

I've moved the uv__tty_console_resize_message_loop_thread start to uv_console_init. This way SIGWINCH signals can be received before any call to uv_tty_init.

The loop in uv__tty_console_resize_message_loop_thread will never be interrupted - it will spin as long as the app is alive. The uv__tty_console_resize_event callback will be run in the uv__tty_console_resize_message_loop_thread thread. Since there is only one such thread no synchronization is needed.

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/364/

bzoz · 2017-08-07T13:20:54Z

CI (with correct branch name): https://ci.nodejs.org/view/libuv/job/libuv-test-commit/368/

bzoz · 2017-08-17T19:51:55Z

Ping?

saghul · 2017-08-18T07:33:37Z

src/win/tty.c

+  MSG msg;
+
+  if (!GetConsoleScreenBufferInfo(uv__tty_console_handle, &sb_info))
+    uv_fatal_error(GetLastError(), "GetConsoleScreenBufferInfo");


Is this really fatal? Can't we live if it fails?

I guess we can, but there is no way to signal the user that this failed, and that SIGWINCH will not work.

I know, but so what? it fails, we move on. I think it's far better than crashing!

saghul · 2017-08-18T07:33:49Z

src/win/tty.c

+                       0,
+                       0,
+                       WINEVENT_OUTOFCONTEXT))
+    uv_fatal_error(GetLastError(), "SetWinEventHook");


saghul · 2017-08-18T07:33:59Z

src/win/tty.c

+                       WINEVENT_OUTOFCONTEXT))
+    uv_fatal_error(GetLastError(), "SetWinEventHook");
+
+  while (GetMessage(&msg, NULL, 0, 0)) {


saghul · 2017-08-18T07:34:13Z

src/win/tty.c

+  int width, height;
+
+  if (!GetConsoleScreenBufferInfo(uv__tty_console_handle, &sb_info))
+    uv_fatal_error(GetLastError(), "GetConsoleScreenBufferInfo");


refack · 2017-08-18T12:30:47Z

[biased scepticism, but I'm open to be persuaded]
I'm not sure this is worth it 🤷‍♂️ upstream we just added a disclaimer nodejs/node@ff07eaa

Do we know of any interesting uses for this event? (honest question)
Anyone that is doing GUI-like console apps? Because very few Windows console apps react to a window change, they keep assuming the initial size and just look horrible:

saghul · 2017-08-18T12:39:48Z

I hate that broken behavior. If we can do better, why not? Also, Node is not the entire universe other projects are not going to read those disclaimers.

bzoz · 2017-08-21T17:53:22Z

@saghul I've removed the uv_fatal_error calls, PTAL

bzoz · 2017-08-24T12:05:59Z

@saghul, @refack PTAL

saghul · 2017-08-25T13:35:46Z

src/win/tty.c

+                          NULL,
+                          WT_EXECUTELONGFUNCTION);
+    if (r == 0)
+      abort();


i don't think this should be fatal

saghul · 2017-08-25T13:36:03Z

src/win/tty.c

 static void uv__determine_vterm_state(HANDLE handle);

 void uv_console_init(void) {
+  int r;


you can declare r inside the if where it's used.

bzoz · 2017-08-29T18:14:59Z

Updated, PTAL

bzoz · 2017-09-06T11:26:45Z

Ping?

saghul

LGTM

Add SetWinEventHook for EVENT_CONSOLE_LAYOUT for better detection of console resize events. Ref: nodejs/node#13197 PR-URL: libuv#1408 Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

bzoz · 2017-09-14T07:32:09Z

CI after rebase: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/460/

bzoz · 2017-09-14T08:25:15Z

Unrelated failures, rerun Windows CI: https://ci.nodejs.org/job/libuv-test-commit-windows/548/.

Add SetWinEventHook for EVENT_CONSOLE_LAYOUT for better detection of console resize events. Ref: nodejs/node#13197 PR-URL: #1408 Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

bzoz · 2017-09-14T08:26:54Z

Landed in 6ad1e81

This commit removes unused variables left over from 6ad1e81. Refs: #1408 PR-URL: #1571 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Continuing improvement of SIGWINCH from PR libuv#2308. Running SetWinEventHook without filtering for the specific PIDs has significant impact on the performance of the entire system. This PR changes the way SIGWINCH is handled. The SetWinEventHook callback now signals a separate thread, uv__tty_console_resize_watcher_thread. This thread calls uv__tty_console_signal_resize() which checks if the console was actually resized. The uv__tty_console_resize_watcher_thread makes sure to not to call the uv__tty_console_signal_resize function more than 30 times per second. The SetWinEventHook will not be installed, if the PID of the conhost.exe process that owns the console window cannot be determinated. This can happen when a 32bit libuv app is running on a 64bit Windows. For such cases PR libuv#1408 is partially reverted - when tty reads WINDOW_BUFFER_SIZE_EVENT, it will also trigger a call to uv__tty_console_signal_resize(). This will also help when the app is running under console emulators. Documentation was alos updated to reflect that. Refs: microsoft/terminal#1811 Refs: microsoft/terminal#410 Refs: libuv#2308

Continuing improvement of SIGWINCH from PR #2308. Running SetWinEventHook without filtering for the specific PIDs has significant impact on the performance of the entire system. This PR changes the way SIGWINCH is handled. The SetWinEventHook callback now signals a separate thread, uv__tty_console_resize_watcher_thread. This thread calls uv__tty_console_signal_resize() which checks if the console was actually resized. The uv__tty_console_resize_watcher_thread makes sure to not to call the uv__tty_console_signal_resize function more than 30 times per second. The SetWinEventHook will not be installed, if the PID of the conhost.exe process that owns the console window cannot be determinated. This can happen when a 32bit libuv app is running on a 64bit Windows. For such cases PR #1408 is partially reverted - when tty reads WINDOW_BUFFER_SIZE_EVENT, it will also trigger a call to uv__tty_console_signal_resize(). This will also help when the app is running under console emulators. Documentation was also updated to reflect that. Refs: microsoft/terminal#1811 Refs: microsoft/terminal#410 Refs: #2308 PR-URL: #2381 Reviewed-By: Ben Noordhuis <[email protected]>

bzoz mentioned this pull request Jul 7, 2017

doc: add note to tty.WriteStream event 'resize' on Windows nodejs/node#13576

Closed

saghul requested changes Jul 24, 2017

View reviewed changes

bzoz force-pushed the bartek-win-sigwinch branch from a805728 to 6418a8a Compare July 28, 2017 10:36

saghul requested changes Jul 29, 2017

View reviewed changes

bzoz force-pushed the bartek-win-sigwinch branch 2 times, most recently from b7e21a5 to 6048b42 Compare August 7, 2017 11:06

saghul requested changes Aug 18, 2017

View reviewed changes

saghul requested changes Aug 25, 2017

View reviewed changes

saghul approved these changes Sep 6, 2017

View reviewed changes

win,tty: improve SIGWINCH support

0b9959e

Add SetWinEventHook for EVENT_CONSOLE_LAYOUT for better detection of console resize events. Ref: nodejs/node#13197 PR-URL: libuv#1408 Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

bzoz force-pushed the bartek-win-sigwinch branch from aa3dd00 to 0b9959e Compare September 14, 2017 07:31

bzoz added a commit that referenced this pull request Sep 14, 2017

win,tty: improve SIGWINCH support

6ad1e81

Add SetWinEventHook for EVENT_CONSOLE_LAYOUT for better detection of console resize events. Ref: nodejs/node#13197 PR-URL: #1408 Reviewed-By: Saúl Ibarra Corretgé <[email protected]>

bzoz closed this Sep 14, 2017

bzoz mentioned this pull request Oct 19, 2017

doc, win: remove note about resize nodejs/node#16320

Closed

2 tasks

digitalinfinity mentioned this pull request Oct 31, 2017

node.exe 8.7.0 and above does not work in Windows NanoServer container nodejs/node#16603

Closed

jblazquez mentioned this pull request May 22, 2019

win: prevent machine hang due to tty event explosion #2308

Closed

bzoz mentioned this pull request Jul 18, 2019

win, tty: improve SIGWINCH performance #2381

Closed

win,tty: improve SIGWINCH support #1408

win,tty: improve SIGWINCH support #1408

Uh oh!

Conversation

bzoz commented Jul 7, 2017

Uh oh!

bzoz commented Jul 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzoz commented Jul 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzoz commented Aug 7, 2017

Uh oh!

bzoz commented Aug 7, 2017

Uh oh!

bzoz commented Aug 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 18, 2017

Uh oh!

saghul commented Aug 18, 2017

Uh oh!

bzoz commented Aug 21, 2017

Uh oh!

bzoz commented Aug 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzoz commented Aug 29, 2017

Uh oh!

bzoz commented Sep 6, 2017

Uh oh!

saghul left a comment

Choose a reason for hiding this comment

Uh oh!

bzoz commented Sep 14, 2017

Uh oh!

bzoz commented Sep 14, 2017

Uh oh!

bzoz commented Sep 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants