test: fix flaky tls-inception

[gibfahn]

PR to address #5386 .

Affected core subsystem(s)

test

Description of change

The test now catches ECONNRESET errors thrown in parallel/test-tls-inception on windows. It also replaces removes socket.destroy() with socket.end(), as socket.destroy() also seemed to be causing occasional failures on windows.

[santigimeno]

Maybe check that the error is ECONNRESET

[MylesBorins]

that seems reasonable to me

[gibfahn]

@santigimeno removing the destination listener doesn't seem to cause any problems on Ubuntu 14.04, but without the error handler I'm still getting the ECONNRESET errors on Win7.

[santigimeno]

@gibm Yes, I was only talking about removing the dest.on('end') but keeping the error listener. Does it work that way?.

[gibfahn]

@santigimeno Yep, just running the test to make sure it's all working, then I'll update this.

[gibfahn]

PR updated as per @santigimeno 's suggestions. Let me know if there's anything else that would be worth adding.

[santigimeno]

Maybe restricting the error handling to windows as in https://github.com/nodejs/node/blob/master/test/parallel/test-child-process-fork-regr-gh-2847.js#L18-L23
Also, probably we can remove the console.error as the error info will be available if the exception is thrown

[santigimeno]

A couple of comments, but LGTM

[gibfahn]

@santigimeno updated, tests still looking good.

[Trott]

Seems like it would be good to have an expert look over even small changes to tls tests. So... /cc @nodejs/crypto

[gibfahn]

Bump!

If anyone else has any problems or suggestions let me know...

[jasnell]

@shigeki @indutny ... could either of you please give this a quick review?

[shigeki]

@jasnell I'm afraid that I don't have Windows to check this PR now. It was crashed several months ago and needs several days to get restored.

[gibfahn]

Is it possible to run the node-test-commit-windows job for this change?

[jasnell]

Full CI run: https://ci.nodejs.org/job/node-test-pull-request/1932/

[gibfahn]

So looking at the test results, it looks like this change causes the test to fail on OSX and SmartOS with ECONNRESET. I can replicate this (at least for OSX) myself.

It looks like the test failure is due to the removal of the dest.on('end') listener. Adding this back in should fix the OSX and SmartOS failures. The Windows test is now passing which is great.

[gibfahn]

Okay, this is interesting.

This works on OSX and fails on Windows with EAADRINUSE:

dest.on('end', function() {
  socket.destroy();
});

This works on Windows and fails on OSX (and possibly SmartOS) with ECONNRESET:

dest.on('end', function() {
  socket.end();
});

Removing this listener entirely also works on Windows (but not OSX).

@santigimeno would you have any ideas about why?

[santigimeno]

@gibm IIRC, the problem I tried to fix in 3b94991 was that sometimes, in OS X, the client socket from the a server emitted an ECONNRESET because of a race condition caused by both ends trying to close the connection at the same time. I tried to improve the situation by calling socket.destroy() asap, but probably the race condition still exists. Maybe there's no way around it and the proper solution is just to have a listener for ECONNRESET error in every platform.

[gibfahn]

@santigimeno okay, that makes sense. So we can:

1. keep the original listener (the one with socket.destroy()), and handle the EADDRINUSE in Windows
2. (as you suggest) remove the whole thing and handle ECONNRESET errors for all platforms (or Mac + Windows + SmartOS).

I don't really know enough about this test to have an opinion on which is better. I'm happy to look at this in more depth, but if anyone else has any more information that'd be helpful.

@indutny I noticed you originally added this handler, do you have any opinions on this?

[jasnell]

What's the status on this one?

[gibfahn]

@jasnell I think it's on hold until someone with more knowledge of this test can take a look, or I get time to go through and decide what makes most sense.

I think it will be one of the options I mentioned above.

[jasnell]

/cc @Trott

[Trott]

/cc @nodejs/platform-windows

[Trott]

LGTM but I'd really like to see at least one Windows or crypto person give it the seal of approval

[gibfahn]

@Trott which change looks good to you? Handling the EADDRINUSE in Windows
or removing the listener and handling ECONNRESET errors for all platforms (or specifically Mac + Windows + SmartOS).

The PR as-is now fails on OSX and SmartOS.

[Trott]

@gibm Never mind, I misunderstood the state of affairs. Sorry for the noise.

[jasnell]

Ping @nodejs/platform-windows @nodejs/testing

[Trott]

Is the difference in behavior on the different platforms just The Way It Is On Those Platforms? Or is it an issue in Node.js (or one of its dependencies like libuv)? Or is that not known at this point?

We certainly have plenty of instances of tests where we check if it's Windows and do one thing, otherwise do something else. So we could go that route here, if people are pretty sure the issue is necessarily different behavior on the different platforms and isn't a bug in Node itself.

/cc @nodejs/crypto

[MylesBorins]

one more ping to @nodejs/crypto and @nodejs/platform-windows

[joaocgreis]

I haven't been able to continue to investigate this one yet, but it's still in my queue.

The best course of action here would be to fix this test with whatever hack it need to test what it's meant to test: the TLS inception, the real goal of the test seems to be the assert at https://github.com/nodejs/node/blob/cdcfeb7/test/parallel/test-tls-inception.js#L64 . However, if I'm not missing anything, none of the destroy or end signals should be handled, because pipe should take care of that. So, the test in its current form minus the end handler should be copied to know issues.

[Trott]

Disregarding the test-on-Windows part of this, can someone in @nodejs/crypto look at this change to this test and comment on whether the change is sensible in that the test still checks that Node.js is behaving correctly? Or does the change here merely cover up buggy behavior and the test should be failing? (And if it's covering up buggy behavior, any opinion as to if it is likely to be a bug in Node.js, libuv, Windows, something else?)

[Trott]

@joaocgreis Unfortunately, I don't think we can copy this over to known_issues because tests in that directory are expected to fail 100% of the time and this is a flaky test that only fails intermittently. :-/

[jasnell]

Ping @gibfahn ... status on this one?

[gibfahn]

To be honest I was shooting in the dark with this one, if someone from @nodejs/platform-windows (maybe @digitalinfinity) could confirm that this makes sense then we can go ahead with this (or not...)

[joaocgreis]

I've been hesitant on this one because I think there's a deeper issue here. At the very least we have a platform discrepancy. The test is testing what it's supposed to test, so I'm +1 with working around the error here (perhaps we should open another issue about the test failing as is now, even without the end/destroy line it should pass on all platforms).

@gibfahn this PR is changing destroy to end, does this work without this change? If the test works on all platforms just by ignoring the error on Windows, I'm +1 for landing this.

[gibfahn]

@joaocgreis Looking at the original issue (#5386) it looks like the ECONNRESET only happens when you change destroy->end, otherwise you get the assertionError instead. So I don't think ignoring the error on windows will help by itself.

I don't think this should land as is, it's really just covering up an existing issue. If we think the test is exposing a real node issue on windows, we shouldn't just try to ignore errors and pretend that everything's fine.

[fhinkel]

@gibfahn is it OK to close this if you say it shouldn't land as is?

[gibfahn]

@fhinkel yep, I'll close.