test: ensure existence of _handle property

[Trott]

Pull Request check-list

Please make sure to review and check all of these items:

• Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
  this change (including linting)?
• Is the commit message formatted according to [CONTRIBUTING.md][0]?
• If this change fixes a bug (or a performance problem), is a regression
  test (or a benchmark) included?
• Is a documentation update included (if this change modifies
  existing APIs, or introduces new ones)?

Affected core subsystem(s)

test

Description of change

test-stdtout-close-unref.js will fail if process.stdin._handle does
not exist. On UNIX-like operating systems, you can see this failure this
way:

./node test/parallel/test-stdout-close-unref.js < /dev/null

This issue has been experienced by @bengl and @drewfish in a Docker
container. I'm not sure why they are experiencing it in their
environment, but since it is possible that the _handle property does
not exist, perhaps it should be checked.

@nodejs/testing

[drewfish]

Thanks for the mention! Yes we're doing our jenkins builds inside of docker images. I'm running the tests using the test-ci makefile target.

(FWIW I've seen another case where node.js was being much more particular about IO streams than other languages.)

[santigimeno]

@Trott is there an issue opened for this? or a way to reproduce it? I have run the test suite on docker from time to time but never saw that error

[Trott]

@santigimeno It's not specific to Docker, although that might be a contributing factor. @bengl asked me about it today and I was able to reproduce the issue by using /dev/null as stdin like this:

./node test/parallel/test-stdout-close-unref.js < /dev/null

[Trott]

Here's what the error looks like without this patch and using /dev/null for stdin as above:

/Users/node/test/parallel/test-stdout-close-unref.js:8
process.stdin._handle.close();
^
TypeError: Cannot read property 'close' of undefined
at Object.<anonymous> (/Users/node/test/parallel/test-stdout-close-unref.js:8:22)
at Module._compile (module.js:413:34)
at Object.Module._extensions..js (module.js:422:10)
at Module.load (module.js:357:32)
at Function.Module._load (module.js:314:12)
at Function.Module.runMain (module.js:447:10)
at startup (node.js:142:18)
at node.js:939:3

[jbergstroem]

Holy crap; I've been seeing this in gentoo in a sandbox for years but couldn't figure out what was going on. Thanks 👍

https://github.com/gentoo/gentoo/blob/master/net-libs/nodejs/nodejs-5.9.0.ebuild#L71..L76

[Trott]

Alternate (perhaps better) solution might be to create a child process with an appropriate stdin and do everything there?

[Fishrock123]

I'm pretty sure there is also an actual... API? issue here too.

I suppose the question would be, should process.stdin always have a ._handle property? I would think yes, but I'm not 100% sure.

<-ing a file into stdin actually results in a fs.ReadStream, rather an a tty.ReadStream, and as such does not inherit from net.Socket, unlike the other possible stdin options:

node/lib/internal/process/stdio.js

Lines 54 to 57 in 293fd04

	case'FILE':
	varfs=require('fs');
	stdin=newfs.ReadStream(null,{fd: fd,autoClose: false});
	break;

[Trott]

OK, I've updated the test so that it doesn't skip if there's no _handle property on process.stdin. Instead, it uses spawn() to set stdin to a pipe to make sure it has a _handle property. PTAL

If the current behavior is deemed a bug as @Fishrock123 seems to suggest, then we can put a version of this test in known_issues that sets stdio to ignore rather than pipe and that will detect the issue.

In the meantime, here's a version of the test that should always pass.

[Fishrock123]

I'm not sure if it is a bug so much as a possible API deficiency. I guess you could just write it off as an oddity but that seems pretty ... not-good to me, although I'm not really sure how to fix it either.

CI: https://ci.nodejs.org/job/node-test-pull-request/2080/

[Fishrock123]

I've made a PR for the known issue at #5935

[Trott]

The single CI failure appears to be known-flaky and unrelated.

[Fishrock123]

lgtm

[Trott]

Any chance we can get a quick confirmation from @drewfish or @bengl that this new patch fixes the test for them? (Because if it doesn't, that's going to be really puzzling...)

[bengl]

Giving it a go. Back soon with results.

[mhdawson]

Good to see this being fixed. The suse build team for PPC was also seeing this failure and I was working with them to try and figure out why.

[bengl]

Yep. Fixes the test for us.

[drewfish]

@jbergstroem do you happen to have also seen failures of test-cluster-master-error.js and test-cluster-master-kill.js? Given that you saw the test-stdtout-close-unref failure as well I thought I'd see how common these other two are before I dig into them.

[Fishrock123]

I suggest doing the following instead:

proc.stderr.pipe(process.stderr);proc.on('exit',common.mustCall(function(exitCode){process.exitCode=exitCode;}));

This way, you can see the child's error, and not need to generate a second error on the parent.

... What I'm going to be doing for TTY testing in #5834 :)

[Trott]

@Fishrock123 Ooh, yes, neat, will do. (I'm going to use process.exit(...) instead of process.exitCode = ... there if it's all the same to you.)

[jbergstroem]

@drewfish sorry for the late response -- I swear I replied (this is the second time I've seen this happen; watching you github!). Anyway, haven't seen any issues with those tests from the gentoo sandbox.

[Trott]

Whoops, pushed the wrong version, let's try again. Updated, PTAL.

CI: https://ci.nodejs.org/job/node-test-pull-request/2103/

[Trott]

CI is being uncooperative. Let's CI again: https://ci.nodejs.org/job/node-test-pull-request/2105/

[Trott]

Well, it was more cooperative that time, but still one host hung while building or something. So let's do it again, because hey, I like my CI green: https://ci.nodejs.org/job/node-test-pull-request/2112/

[Trott]

@Fishrock123 Your LGTM still stands?

[Fishrock123]

I would probably make this just set the process.exitCode, rather than exit().

exit() is a bit ... expedited ... and can cause unwanted things. :)

[Trott]

@Fishrock123 I suppose so. I did that instead of setting process.exitCode because setting the exit code but doing nothing else is kind of magical and also leaves open the possibility of someone adding a test to this file later on that does the same thing, thus overriding the exit code here. I'm probably overthinking it, though. Will switch to your recommendation.

[Fishrock123]

lgtm otherwise

[Trott]

Updated to use process.exitCode if the spawned process exits with a non-zero code.

[Trott]

CI for good measure: https://ci.nodejs.org/job/node-test-pull-request/2116/

[Trott]

Landed in a20c700