test: fix flaky test-http-chunked-304 on SmartOS #3903

claudiorodriguez · 2015-11-18T19:03:25Z

SmartOS has an issue where it will trigger ECONNREFUSED when it
should not. See https://smartos.org/bugview/OS-2767.

This change adds logic to test-http-chunked-304 to work around
the issue. See also similar issue: #2663

Fixes: #3864

Trott · 2015-11-18T22:09:37Z

~~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/

indutny · 2015-11-18T22:12:34Z

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

Trott · 2015-11-18T22:15:47Z

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

Trott · 2015-11-18T22:17:15Z

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)

indutny · 2015-11-18T22:20:15Z

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?

claudiorodriguez · 2015-11-18T22:44:48Z

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.

Trott · 2015-11-18T22:59:38Z

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.

claudiorodriguez · 2015-11-18T23:15:31Z

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

Trott · 2015-11-19T05:22:51Z

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

Trott · 2015-11-19T06:28:54Z

test/parallel/test-http-chunked-304.js

@@ -24,20 +24,39 @@ function test(statusCode, next) {
  });

  server.listen(common.PORT, function() {
-    var conn = net.createConnection(common.PORT, function() {
-      conn.write('GET / HTTP/1.1\r\n\r\n');
+    var 


Linting fails due to trailing space on this line. Can you fix that and use make jslint to confirm no other oddities slipped in?

Trott · 2015-11-19T06:33:06Z

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

SmartOS has an issue where it will trigger ECONNREFUSED when it should not. See https://smartos.org/bugview/OS-2767. This change adds logic to test-http-chunked-304 to work around the issue. See also similar issue: nodejs#2663 Fixes: nodejs#3864

claudiorodriguez · 2015-11-19T12:00:35Z

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

indutny · 2015-11-19T14:41:07Z

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

Trott · 2015-11-19T16:07:40Z

@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?

indutny · 2015-11-19T16:33:13Z

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

claudiorodriguez · 2015-11-19T17:55:59Z

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).

indutny · 2015-11-19T19:16:58Z

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

claudiorodriguez · 2015-11-19T19:44:43Z

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.

indutny · 2015-11-19T19:55:21Z

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

claudiorodriguez · 2015-11-19T20:24:33Z

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?

Trott · 2015-11-19T23:13:18Z

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.

indutny · 2015-11-19T23:27:36Z

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

Trott · 2015-11-20T06:34:25Z

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

indutny · 2015-11-20T06:36:11Z

@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.

Trott · 2015-11-20T16:44:22Z

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

indutny · 2015-11-20T17:36:33Z

Exactly! ;)

Trott · 2015-11-20T18:22:08Z

#3941

claudiorodriguez · 2015-11-20T18:40:56Z

Closing this, then.

mscdex added http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. smartos Issues and PRs related to the SmartOS platform. labels Nov 18, 2015

claudiorodriguez force-pushed the test-flaky-http-chunked-smartos branch from e7efad6 to ade9bfb Compare November 18, 2015 20:39

claudiorodriguez force-pushed the test-flaky-http-chunked-smartos branch from ade9bfb to 210b623 Compare November 18, 2015 23:11

Trott reviewed Nov 19, 2015
View reviewed changes

claudiorodriguez force-pushed the test-flaky-http-chunked-smartos branch from 210b623 to 89d67c3 Compare November 19, 2015 11:59

claudiorodriguez closed this Nov 20, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix flaky test-http-chunked-304 on SmartOS #3903

test: fix flaky test-http-chunked-304 on SmartOS #3903

claudiorodriguez commented Nov 18, 2015

Trott commented Nov 18, 2015

indutny commented Nov 18, 2015

Trott commented Nov 18, 2015

Trott commented Nov 18, 2015

indutny commented Nov 18, 2015

claudiorodriguez commented Nov 18, 2015

Trott commented Nov 18, 2015

claudiorodriguez commented Nov 18, 2015

Trott commented Nov 19, 2015

Trott Nov 19, 2015

Trott commented Nov 19, 2015

claudiorodriguez commented Nov 19, 2015

indutny commented Nov 19, 2015

Trott commented Nov 19, 2015

indutny commented Nov 19, 2015

claudiorodriguez commented Nov 19, 2015

indutny commented Nov 19, 2015

claudiorodriguez commented Nov 19, 2015

indutny commented Nov 19, 2015

claudiorodriguez commented Nov 19, 2015

Trott commented Nov 19, 2015

indutny commented Nov 19, 2015

Trott commented Nov 20, 2015

indutny commented Nov 20, 2015

Trott commented Nov 20, 2015

indutny commented Nov 20, 2015

Trott commented Nov 20, 2015

claudiorodriguez commented Nov 20, 2015

test: fix flaky test-http-chunked-304 on SmartOS #3903

test: fix flaky test-http-chunked-304 on SmartOS #3903

Conversation

claudiorodriguez commented Nov 18, 2015

Trott commented Nov 18, 2015

indutny commented Nov 18, 2015

Trott commented Nov 18, 2015

Trott commented Nov 18, 2015

indutny commented Nov 18, 2015

claudiorodriguez commented Nov 18, 2015

Trott commented Nov 18, 2015

claudiorodriguez commented Nov 18, 2015

Trott commented Nov 19, 2015

Trott Nov 19, 2015

Choose a reason for hiding this comment

Trott commented Nov 19, 2015

claudiorodriguez commented Nov 19, 2015

indutny commented Nov 19, 2015

Trott commented Nov 19, 2015

indutny commented Nov 19, 2015

claudiorodriguez commented Nov 19, 2015

indutny commented Nov 19, 2015

claudiorodriguez commented Nov 19, 2015

indutny commented Nov 19, 2015

claudiorodriguez commented Nov 19, 2015

Trott commented Nov 19, 2015

indutny commented Nov 19, 2015

Trott commented Nov 20, 2015

indutny commented Nov 20, 2015

Trott commented Nov 20, 2015

indutny commented Nov 20, 2015

Trott commented Nov 20, 2015

claudiorodriguez commented Nov 20, 2015