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] process.stdin.unref doesn't work #22999

Closed
ghost opened this issue Sep 21, 2018 · 7 comments
Closed

[Windows] process.stdin.unref doesn't work #22999

ghost opened this issue Sep 21, 2018 · 7 comments
Labels
libuv Issues and PRs related to the libuv dependency or the uv binding. windows Issues and PRs related to the Windows platform.

Comments

@ghost
Copy link

ghost commented Sep 21, 2018

  • Version: v10.10.0
  • Platform: Windows x64

parent.js

require('child_process').spawn('node', ['child.js'], {
  stdio: 'inherit',
});

child.js

const doNothing = () => {};
process.stdin.on('data', doNothing);

require('child_process').spawn('node', ['-v']).on('exit', () => {
  process.stdin.unref();
  console.log('After printing this, the process should exit');
});

Run with node parent.js

Results

Linux: The process terminates by itself
Mac: The process terminates by itself
Windows: The process keeps running until Enter is pressed

This strange behavior seems to break some npm modules.

Other details

Adding process.stdin.removeListener('data', doNothing)
or process.stdin.removeAllListeners('data')
before unref doesn't make any difference.

When enter is pressed, it doesn't trigger data callback, just terminates the process.


Edit

Simplified code:

// parent.js
process.stdin.on('data', () => {});
setTimeout(() => {
  process.stdin.unref();
  console.log('After printing this, the process should exit');
});

Here we don't have a child process. Run with node parent.js

Results on windows

50% of times - the process exits
50% of times - the process keeps running

@lovinglyy
Copy link
Contributor

The simplified code is always working fine for me, the first one I can reproduce that it's needed to press enter on Windows. Node v10.10.0 also.

@bzoz
Copy link
Contributor

bzoz commented Sep 24, 2018

Can reproduce with v10, does not reproduce with v8.

@bzoz
Copy link
Contributor

bzoz commented Sep 25, 2018

This is caused by #21528

/cc @addaleax

@addaleax addaleax added windows Issues and PRs related to the Windows platform. libuv Issues and PRs related to the libuv dependency or the uv binding. labels Sep 25, 2018
@addaleax
Copy link
Member

That most likely means that the streams implementation for Windows does not clean up fully when uv_close() is used without an extra uv_read_stop() that precedes it.

We can revert #21528 without any issues, but the real issue is almost certainly a libuv one.

@bzoz
Copy link
Contributor

bzoz commented Sep 26, 2018

Ok, its a libuv bug. I have a fix, I will open a PR with a fix soon.

bzoz added a commit to JaneaSystems/libuv that referenced this issue Sep 27, 2018
Under some condition, uv_tty_close would not stop console reads. This
fixes that issue by first stopping read, then closing the handle.

Ref: nodejs/node#22999
@bzoz
Copy link
Contributor

bzoz commented Sep 27, 2018

Fix in libuv/libuv#2005

@cjihrig
Copy link
Contributor

cjihrig commented Sep 30, 2018

Reopening until the next libuv update.

@cjihrig cjihrig reopened this Sep 30, 2018
cjihrig added a commit to cjihrig/node that referenced this issue Oct 8, 2018
@Trott Trott closed this as completed in c65a523 Oct 9, 2018
targos pushed a commit that referenced this issue Oct 10, 2018
PR-URL: #23336
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Fixes: #23043
Fixes: #21773
Fixes: #16601
Fixes: #22999
Fixes: #23219
Fixes: #23066
Fixes: #23067
Fixes: #23089
jasnell pushed a commit that referenced this issue Oct 17, 2018
PR-URL: #23336
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Fixes: #23043
Fixes: #21773
Fixes: #16601
Fixes: #22999
Fixes: #23219
Fixes: #23066
Fixes: #23067
Fixes: #23089
MylesBorins pushed a commit to MylesBorins/node that referenced this issue Nov 5, 2018
PR-URL: nodejs#23336
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Fixes: nodejs#23043
Fixes: nodejs#21773
Fixes: nodejs#16601
Fixes: nodejs#22999
Fixes: nodejs#23219
Fixes: nodejs#23066
Fixes: nodejs#23067
Fixes: nodejs#23089
MylesBorins pushed a commit that referenced this issue Nov 11, 2018
Backport-PR-URL: #24103
PR-URL: #23336
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Fixes: #23043
Fixes: #21773
Fixes: #16601
Fixes: #22999
Fixes: #23219
Fixes: #23066
Fixes: #23067
Fixes: #23089
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libuv Issues and PRs related to the libuv dependency or the uv binding. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants