fix flaky net-connect-handle-econnrefused #18257

Leko · 2018-01-19T15:48:49Z

This PR closes #18164

I changed timing of connecting to server.
It connects to server after server.close called certainly.

Does that solve this issue?
I'd appreciate any feedback or suggestions.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

cjihrig · 2018-01-19T19:53:35Z

test/parallel/test-net-connect-handle-econnrefused.js

-  assert.strictEqual('ECONNREFUSED', e.code);
-}));
-server.close();
+server.close(() => {


This test seems a bit racey in general. If you wait for the server to close, couldn't another process potentially take the port?

@cjihrig Hmm... You're right.
I think better is move test-net-connect-handle-econnrefused.js to test/sequential.
If this test run sequentially, another test cannot take the port, right?

I would stick to having it be in parallel and seeing if this fixes it. We need less tests in sequential, not more, as they take a really long time to run.

joyeecheung

LGTM. The test itself is racy but at least this change make it somewhat less racy...

joyeecheung · 2018-01-20T01:18:40Z

test/parallel/test-net-connect-handle-econnrefused.js

-  assert.strictEqual('ECONNREFUSED', e.code);
-}));
-server.close();
+server.close(() => {


Can you wrap this in common.mustCall?

@joyeecheung It would be nice. I'll fix it.

targos · 2018-01-21T10:59:55Z

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

lpinca · 2018-01-21T20:08:15Z

I agree with @cjihrig, this does not fix potential race conditions that could happen if another process grabs the port before the close() callback is invoked.

BridgeAR · 2018-02-01T16:57:12Z

In general I agree that we need less tests in sequential but having a racy test is bad as well.

We should probably have a look at our sequential tests in general. I am going to open a meta issue for that.

@apapirovski are you good with this for now?

BridgeAR · 2018-02-06T22:12:38Z

Ping @apapirovski

apapirovski · 2018-02-06T22:16:50Z

Am I holding this up? I approved this as is, with no changes or nits. My comments was re: keeping it in parallel which it is. I don't think I should be the blocker?

If there's concerns, I would suggest running a stress test with -j 96 --repeat 192 perhaps and seeing what comes back?

BridgeAR · 2018-02-06T22:19:42Z

My point was that I actually think it is better to be on the safe side right now. Our CI is really bad at the moment because we have so many flakes and it would be great to reduce those.

Working on the sequential ones at a different time to reduce those would for me be the better approach.

apapirovski · 2018-02-06T22:24:49Z

Stress test CI: https://ci.nodejs.org/job/node-stress-single-test/1776/ and running one locally. If either doesn't pass then we should move to sequential.

No failures locally after many runs but the stress test job is always more flaky so that should ultimately decide.

BridgeAR · 2018-02-16T18:13:10Z

Landed in befe675 🎉

PR-URL: nodejs#18257 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #18257 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs#18257 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: #18257 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

The port used in the test could be taken by another process before the callback of `server.close()` is called. Move it to sequential. Refs: nodejs#18257 (comment) Fixes: nodejs#26907

The port used in the test could be taken by another process before the callback of `server.close()` is called. Move it to sequential. PR-URL: nodejs#27014 Fixes: nodejs#26907 Refs: nodejs#18257 (comment) Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

The port used in the test could be taken by another process before the callback of `server.close()` is called. Move it to sequential. PR-URL: #27014 Fixes: #26907 Refs: #18257 (comment) Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

test: try to connect after server was closed

a5965e5

nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 19, 2018

cjihrig reviewed Jan 19, 2018

View reviewed changes

joyeecheung approved these changes Jan 20, 2018

View reviewed changes

targos approved these changes Jan 20, 2018

View reviewed changes

apapirovski approved these changes Jan 20, 2018

View reviewed changes

test: wrap callback in common.mustCall

515e7d8

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

jasnell approved these changes Feb 10, 2018

View reviewed changes

BridgeAR approved these changes Feb 13, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 13, 2018

BridgeAR closed this Feb 16, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

Leko deleted the fix_flaky_net_econnrefused branch February 26, 2018 13:25

apapirovski mentioned this pull request Apr 12, 2018

flaky net-connect-handle-econnrefused #18164

Closed

MylesBorins added dont-land-on-v8.x labels Aug 7, 2018

MylesBorins removed the dont-land-on-v8.x label Aug 7, 2018

MylesBorins mentioned this pull request Aug 16, 2018

v8.12.0 proposal #21593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix flaky net-connect-handle-econnrefused #18257

fix flaky net-connect-handle-econnrefused #18257

Leko commented Jan 19, 2018

cjihrig Jan 19, 2018

Leko Jan 20, 2018

apapirovski Jan 20, 2018

joyeecheung left a comment

joyeecheung Jan 20, 2018

Leko Jan 20, 2018

targos commented Jan 21, 2018

lpinca commented Jan 21, 2018

BridgeAR commented Feb 1, 2018

BridgeAR commented Feb 6, 2018

apapirovski commented Feb 6, 2018

BridgeAR commented Feb 6, 2018

apapirovski commented Feb 6, 2018 •

edited

Loading

BridgeAR commented Feb 16, 2018

fix flaky net-connect-handle-econnrefused #18257

fix flaky net-connect-handle-econnrefused #18257

Conversation

Leko commented Jan 19, 2018

Checklist

Affected core subsystem(s)

cjihrig Jan 19, 2018

Choose a reason for hiding this comment

Leko Jan 20, 2018

Choose a reason for hiding this comment

apapirovski Jan 20, 2018

Choose a reason for hiding this comment

joyeecheung left a comment

Choose a reason for hiding this comment

joyeecheung Jan 20, 2018

Choose a reason for hiding this comment

Leko Jan 20, 2018

Choose a reason for hiding this comment

targos commented Jan 21, 2018

lpinca commented Jan 21, 2018

BridgeAR commented Feb 1, 2018

BridgeAR commented Feb 6, 2018

apapirovski commented Feb 6, 2018

BridgeAR commented Feb 6, 2018

apapirovski commented Feb 6, 2018 • edited Loading

BridgeAR commented Feb 16, 2018

apapirovski commented Feb 6, 2018 •

edited

Loading