If CI is happy, this LGTM. EDIT: Stress test CI is not happy.

/cc @indutny since he wrote the test and it has had no substantial modification by anyone else until now.

CI stress test with this change:

• https://ci.nodejs.org/job/node-stress-single-test/21/nodes=smartos14-32/console

CI stress test without this change (should show failures):

• Not running yet, but I'll edit this when it is running. Will probably be https://ci.nodejs.org/job/node-stress-single-test/22/nodes=smartos14-32/console but right now that just redirects to the above

CI for this PR:

• https://ci.nodejs.org/job/node-test-pull-request/774/

What exactly happens on SmartOS? How could we get ECONNREFUSED after starting the server?

@indutny: Does the explanation at the bottom of https://smartos.org/bugview/OS-2767 offer a plausible mechanism for spurious ECONNREFUSED in this situation?

Bad news anyway: The stress test still gets failures with this fix in place.

events.js:141
      throw er; // Unhandled 'error' event
      ^

Error: connect ECONNREFUSED 127.0.0.1:12346
    at Object.exports._errnoException (util.js:915:11)
    at exports._exceptionWithHostPort (util.js:938:20)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1065:14)

Gosh, really? I think most of the tests should be broken on SmartOS then. Perhaps we should mark the whole platform as flaky, or restart tests on ECONNRESET?

Hmmm. Now that I'm at home and looking at the code, it kinda makes sense to me that, since we're talking about "connection refused", the "connect" event isn't called at all in that case, and since the "error" listener only gets bound inside the listener... yeah. Let me just make another change. I wish this issue was easier to test.

OK, in that case, I'm going to terminate the running CI because it's tying up other tests that are waiting for a 32-bit SmartOS machine.

Yeah, good call. Just made a commit, I'd say if a fail comes up, also terminate it and look for an alternate solution.

CI stress test for new version: https://ci.nodejs.org/job/node-stress-single-test/23/nodes=smartos14-32/console

Stress test looks good. Can you fix the minor linting problem?

Oops, sorry, still getting used to the project. Fixed and jslint is coming up clear.

@Trott why do we want to land this? Why other tests are not failing with ECONNRESET?

@indutny I'll take your second question first:

Why other tests are not failing with ECONNRESET?

(It's ECONNREFUSED rather than ECONNRESET.)

We definitely see it on other tests. In #2663 we saw it a lot on test-net-server-max-connections.js because that test opens hundreds of connections. It was fixed in #3830.

It doesn't pop up as often on other tests probably because they typically only open a few connections and not hundreds. But we do see it.

Here are two more occurrences on different tests since this test was designated flaky a few days ago.

• https://ci.nodejs.org/job/node-test-commit-smartos/364/nodes=smartos14-64/console

not ok 370 test-http-flush-headers.js
#events.js:141
#      throw er; // Unhandled 'error' event
#      ^
#
#Error: connect ECONNREFUSED 127.0.0.1:12346
#    at Object.exports._errnoException (util.js:872:11)
#    at exports._exceptionWithHostPort (util.js:895:20)
#    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1065:14)

• https://ci.nodejs.org/job/node-test-commit-smartos/338/nodes=smartos14-64/console

not ok 522 test-net-dns-custom-lookup.js
#events.js:141
#      throw er; // Unhandled 'error' event
#      ^
#
#Error: connect ECONNREFUSED 127.0.0.1:12346
#    at Object.exports._errnoException (util.js:915:11)
#    at exports._exceptionWithHostPort (util.js:938:20)
#    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1065:14)

why do we want to land this?

It's a way to let the test not be flaky on SmartOS. I admit that part of it doesn't pass the smell test for me. The retry logic seemed OK in the max-server-connections test mentioned above, but to move it to a bunch of other tests... ¯\_(ツ)_/¯ I'm certainly open to other ideas. Things I've thought of:

• I'm not sure how SmartOS is versioned and how EOL works with it and all, but we could decide we don't support the OS/hardware combinations where we see this problem popping up and stop testing on it.
• Or we could just skip the flaky tests on SmartOS rather than introducing the more convoluted retry logic we're introducing here.
• Maybe there's an OS tweak we could do that will make this problem go away. SmartOS is a Joyent thing, right? Is there someone we can refer this issue to?
• We could mark the tests as flaky on SmartOS with no intention of making them not-flaky ever. I don't like that option, but it is an option.

Other ideas?

Or we could just skip the flaky tests on SmartOS rather than introducing the more convoluted retry logic we're introducing here.

This is what I propose cc @bnoordhuis

Even if we go that way I'd suggest keeping the change that extracts the listener bindings out of the connect listener, and also keeping the error handling - basically everything in this PR except the reconnect logic (on a new PR).

What's the reasoning of this @fansworld-claudio ? The test appears to be working fine everywhere else.

Making it easier to implement future changes I guess, mainly. It's true that it works right now, in the context it's executed. I guess not then.

Well, it is a test, so I'm not sure if future-proofing it is reasonable. Sorry for this!

Don't worry, like I said I'm still getting used to the project and it makes perfect sense.
On the subject of skipping the tests, I agree that the reconnect logic is kind of an ugly workaround, but skipping an entire platform also seems less than ideal. Is it possible to narrow down the criteria a bit?

Unfortunately, we have two SmartOS builds in the test CI and both of them are affected. This would seem to only affect tests where a server is set up and then an attempt is made to connect to that server. That's a lot of tests under net and http, but I wouldn't want to skip any of them prematurely because there are probably only certain conditions that trip the bug. So maybe we just add the standard three-or-four lines of skip code at the beginning of tests if the test:

• fails with ECONNREFUSED for no apparent reason on CI and
• we run node-stress-single-test to confirm that the flakiness

The standard three-or-four lines I'm talking about look like this:

if (common.isSunOS) {
  console.log('1..0 # Skipped: Reason for skipping here.');
  return;
}

This will be a lot of tests in the end, and we will want to be able to roll them all back should the bug get fixed in SmartOS. So we'll want to make sure the reason provided for skipping is identical in all the tests so that we can find them easily.

We could just retry test several times if it fails with ECONNREFUSED, maybe 2 or 3 times before failing

Retrying the test if we're on SmartOS and get an ECONNREFUSED is basically what this PR does, isn't it?

@Trott I mean doing this automatically on CI, regardless of particular test name. Again, there are lots of tests with similar semantics, and doing this hack for all of them seems to be pointless to me.

That would probably mean putting the logic for it in the Python test wrapper/harness. That should work.

Exactly! ;)

#3941

Closing this, then.