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

Fix discardStdin option #135

Merged
merged 6 commits into from Nov 13, 2019
Merged

Fix discardStdin option #135

merged 6 commits into from Nov 13, 2019

Conversation

stroncium
Copy link
Contributor

@stroncium stroncium commented Oct 5, 2019

Disable discardStdin option on Windows. BTW, when using cmd nothing is printed anyway on Windows(unless in raw mode I suppose). Fixes #132, fixes #134, fixes #136.

Problem mostly boiled down to using raw mode twice(on separate ticks) hanged the problem under windows. Relevant change added to example.js(tested in cmd).

Make discarding work with multiple parallel ora instances, fixes #133.

#134 is a problem under macs too, looking into it right now.

@stroncium
Copy link
Contributor Author

stroncium commented Oct 5, 2019

I'll be keeping some notes regarding the multiple issues here.

Starting with:
The windows hanging problem seems to be a bug in nodejs. I found a few similar issues against nodejs and libuv, most of them fixed, not sure if all.

However, here is the minimal reproduction:

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

This code alone hangs event loop. I don't know if it's expected behavior, the documentation on that is somewhat vague. resume and pause are supposed to be triggered by this actions(judging from docs and code), but they only work for the first time this is done under linux. (Under windows pause doesn't seem to be called at all.)
When pause and resume are called manually, everything seems to work fine under linux, but under windows it produces bizarre state(not sure if event loop hang or something else).
With some modifications(need to figure out what modifications exactly, but it seems it have to do with manually calling pause and resume), it hangs it so much that proccess.exit(0) and stdin.unref() stop working. I didn't check that far, but I suppose that's the same probIem as described further.

@stroncium
Copy link
Contributor Author

stroncium commented Oct 5, 2019

Hangs under windows.
Works under linux.

const readline = require('readline');

const input = process.stdin;
const output = process.stdout;

let rl = readline.createInterface({input, output});
setTimeout(() => { // causes no problem if ran on process.nextTick or on the same tick
	rl.close();
	let rl2 = readline.createInterface({input, output}); //hangs
	process.exit(0); // this line will never be executed
}, 0);

@stroncium
Copy link
Contributor Author

stroncium commented Oct 5, 2019

@sindresorhus What do you think about just disabling this feature on Windows?

I've spent a few days debugging it but I still fail to produce even minimal reproduction(a lot of different actions seem to trigger it). My current understanding is that the problem is either entirely in libuv or in a state of std streams produced by some chain of actions which triggers the problem in libuv. Further debugging would require me to set up a whole compiler chain and debugger for windows(a couple more full days based on my experience) and dig into what happens on libuv level(undetermined amount of time), so doesn't seem to be worth it. Also, given the current state there is a high chance even if I fix it it will get broken again with more node updates.

@sindresorhus
Copy link
Owner

What do you think about just disabling this feature on Windows?

Yes, sounds good. I just want to get out a fix. We can open an issue afterwards in case anyone wants to look into adding Windows support to the option.

@sorenlouv
Copy link

Anything blocking this PR from being merged?

@sindresorhus
Copy link
Owner

sindresorhus commented Nov 12, 2019

@stroncium Would you be able to finish this by just disabling it on Windows?

@stroncium
Copy link
Contributor Author

@sindresorhus Yeah,just figured you were swamped with PRs and took a bit more time to investigate(nothing helpful tho, so sticking with disabling for windows still).

@stroncium stroncium changed the title [WIP] Fix discardStdin option Fix discardStdin option Nov 12, 2019
@stroncium stroncium changed the title Fix discardStdin option [WIP] Fix discardStdin option Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants