test: fix `net-socket-timeout-unref` flakiness #4772

santigimeno · 2016-01-19T22:17:30Z

From time to time this test is failing in OS X because at least one of
the connections takes quite a long time (around 5 seconds) causing some
of the timers may fire before the test exited. To solve this, wait for
all the connections to be established before setting the timeouts and
unrefing the sockets.

The error I'm getting is

=== release test-net-socket-timeout-unref ===                                  
Path: parallel/test-net-socket-timeout-unref
[]
[]
[]
[]
assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: Socket timeout should not hold loop open
    at process.<anonymous> (/Users/sgimeno/node/node/test/parallel/test-net-socket-timeout-unref.js:26:10)
    at emitOne (events.js:96:20)
    at process.emit (events.js:183:7)

I don't know if it's a legit issue or just a problem in my OS X machine as it seems strange that some connection takes that long, but I have seen this error appear relatively frequently when running the test suite and I can reproduce it more easily by running multiple copies of this test in parallel.

jasnell · 2016-01-22T04:41:30Z

/cc @Trott @nodejs/testing

r-52 · 2016-01-31T18:32:14Z

@santigimeno saw it also on my OS X machine, but it's really really rare.

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

Trott · 2016-02-03T05:20:25Z

Just had this test fail on me in CI for the first time I can remember:

https://ci.nodejs.org/job/node-test-binary-arm/926/RUN_SUBSET=0,nodes=pi1-raspbian-wheezy/console

Trott · 2016-02-03T05:27:30Z

LGTM and CI is green. Any thoughts or concerns @jasnell @romankl @nodejs/testing ? I'd like at least one more LGTM besides my own.

cjihrig · 2016-02-03T15:05:24Z

test/parallel/test-net-socket-timeout-unref.js

  });
-  socket.unref();
+
+  sockets.push([ socket, T ]);


Can you remove the whitespace around the braces.

Enable `space-unary-ops` in `.eslintrc`. This prohibits things like: i ++ // use `i++` instead typeof(foo) // use `typeof foo` or `typeof (foo)` instead Ref: nodejs#4772 (comment)

santigimeno · 2016-02-03T22:10:44Z

PR update. Thanks for the review.

cjihrig · 2016-02-03T22:16:00Z

test/parallel/test-net-socket-timeout-unref.js

-    timedout = true;
-    socket.destroy();
+  socket.on('connect', function() {
+    if (++connections === 6) {


Can you remove the hard coded 6. Maybe use the array length.

cjihrig · 2016-02-03T22:16:23Z

One comment. LGTM if the CI is happy.

From time to time this test is failing in OS X because at least one of the connections takes quite a long time (around 5 seconds) causing some of the timers may fire before the test exited. To solve this, wait for all the connections to be established before setting the timeouts and unrefing the sockets.

santigimeno · 2016-02-03T23:46:40Z

Sorry, I forgot about that. Done

Trott · 2016-02-04T03:45:07Z

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

jasnell · 2016-02-04T18:02:57Z

LGTM

From time to time this test is failing in OS X because at least one of the connections takes quite a long time (around 5 seconds) causing some of the timers may fire before the test exited. To solve this, wait for all the connections to be established before setting the timeouts and unrefing the sockets. PR-URL: #4772 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2016-02-04T18:04:00Z

Landed in 538813c

Enable `space-unary-ops` in `.eslintrc`. This prohibits things like: i ++ // use `i++` instead typeof(foo) // use `typeof foo` or `typeof (foo)` instead Ref: #4772 (comment) PR-URL: #5063 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: James M Snell <jasnell@gmail.com>