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

resize event not firing #16194

Closed
Flet opened this issue Oct 13, 2017 · 7 comments · Fixed by #16225
Closed

resize event not firing #16194

Flet opened this issue Oct 13, 2017 · 7 comments · Fixed by #16225
Labels
confirmed-bug Issues with confirmed bugs. macos Issues and PRs related to the macOS platform / OSX. regression Issues related to regressions. tty Issues and PRs related to the tty subsystem.

Comments

@Flet
Copy link

Flet commented Oct 13, 2017

  • Version: v8.5.0 through v8.7.0
  • Platform: Darwin
  • Subsystem:

In versions v8.5.0 and higher, the resize event does not seem to fire on terminal window resize.

Test code:

process.stdout.on('resize', function () {
  console.log('resized')
})

setInterval(function () {
}, 0)

I was able to have success when trying v8.4.0, but v8.5.0 does not work.

Possibly related recent closed issue:
#13197

@refack
Copy link
Contributor

refack commented Oct 13, 2017

/cc @nodejs/platform-macos

@Flet #13197 would have been my first guess too, but it was closed by a documentation change - ff07eaa
Later the underlying event mechanism was improved in libuv/libuv@6ad1e81 but that change was for Windows only, and your report states Darwin as the platform.

@refack refack added macos Issues and PRs related to the macOS platform / OSX. tty Issues and PRs related to the tty subsystem. labels Oct 13, 2017
@bnoordhuis
Copy link
Member

I'm not sure what broke exactly but it starts working again when you add a no-op process.on('SIGWINCH', () => {}) listener. Manual bisecting indicates it's not the libuv upgrade.

lib/internal/process/stdio.js installs a SIGWINCH handler but it doesn't fire until another one is added because the 'newListener' handler for signals hasn't been installed yet.

My initial guess was that some of the inspector changes broke it because the problem goes away with this patch but I haven't pinpointed the cause yet:

diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js
index e7676c0867..b9ac545dd1 100644
--- a/lib/internal/bootstrap_node.js
+++ b/lib/internal/bootstrap_node.js
@@ -36,6 +36,7 @@
     const browserGlobals = !process._noBrowserGlobals;
     if (browserGlobals) {
       setupGlobalTimeouts();
+      _process.setupSignalHandlers();
       setupGlobalConsole();
     }

@@ -57,7 +58,6 @@
     _process.setup_cpuUsage();
     _process.setupMemoryUsage();
     _process.setupKillAndExit();
-    _process.setupSignalHandlers();
     if (global.__coverage__)
       NativeModule.require('internal/process/write-coverage').setup();

@bnoordhuis bnoordhuis added the confirmed-bug Issues with confirmed bugs. label Oct 14, 2017
@maclover7
Copy link
Contributor

Did a bisect, and it seems like f34e0f9 is what introduced the new behavior.

@refack refack added the regression Issues related to regressions. label Oct 14, 2017
@refack
Copy link
Contributor

refack commented Oct 14, 2017

@maclover7 , @Flet can you devise a regression test for this? (something like: start a child node process in a terminal, then try to script a resize of that terminal)

Another good test to have is a unit test of the event on the node side (one that mocks the libuv behaviour)

@bnoordhuis
Copy link
Member

Did a bisect, and it seems like f34e0f9 is what introduced the new behavior.

That commit was one of my suspects but its code does not exist anymore in v8.6.0, it was removed in 8ce0e9a (which is a somewhat suspect commit as well.)

@Fishrock123
Copy link
Contributor

Fishrock123 commented Oct 15, 2017

This sounds reasonable to me, we should set up the signal handlers before loading the console / process.{stdio}.

@bnoordhuis
Copy link
Member

#16225

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Nov 15, 2017
It's not wholly clear what commit introduced the regression but between
v8.4.0 and v8.5.0 the 'resize' event stopped getting emitted when the
tty was resized.

The SIGWINCH event listener apparently was being installed before the
support code for `process.on('SIGWINCH', ...)` was.  Fix that by moving
said support code to real early in the bootstrap process.

This commit also seems to fix a Windows-only "write EINVAL" error for
reasons even less well-understood...

Fixes: nodejs#16141
Fixes: nodejs#16194
PR-URL: nodejs#16225
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 11, 2017
It's not wholly clear what commit introduced the regression but between
v8.4.0 and v8.5.0 the 'resize' event stopped getting emitted when the
tty was resized.

The SIGWINCH event listener apparently was being installed before the
support code for `process.on('SIGWINCH', ...)` was.  Fix that by moving
said support code to real early in the bootstrap process.

This commit also seems to fix a Windows-only "write EINVAL" error for
reasons even less well-understood...

Fixes: #16141
Fixes: #16194
PR-URL: #16225
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
gibfahn pushed a commit that referenced this issue Dec 20, 2017
It's not wholly clear what commit introduced the regression but between
v8.4.0 and v8.5.0 the 'resize' event stopped getting emitted when the
tty was resized.

The SIGWINCH event listener apparently was being installed before the
support code for `process.on('SIGWINCH', ...)` was.  Fix that by moving
said support code to real early in the bootstrap process.

This commit also seems to fix a Windows-only "write EINVAL" error for
reasons even less well-understood...

Fixes: #16141
Fixes: #16194
PR-URL: #16225
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. macos Issues and PRs related to the macOS platform / OSX. regression Issues related to regressions. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants