Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix flaky net-connect-handle-econnrefused #18257

Closed
wants to merge 2 commits into from
Closed

fix flaky net-connect-handle-econnrefused #18257

wants to merge 2 commits into from

Conversation

Leko
Copy link
Contributor

@Leko Leko commented Jan 19, 2018

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 19, 2018
assert.strictEqual('ECONNREFUSED', e.code);
}));
server.close();
server.close(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assert.strictEqual('ECONNREFUSED', e.code);
}));
server.close();
server.close(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you wrap this in common.mustCall?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@targos
Copy link
Member

targos commented Jan 21, 2018

@lpinca
Copy link
Member

lpinca commented Jan 21, 2018

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
Copy link
Member

BridgeAR commented Feb 1, 2018

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
Copy link
Member

BridgeAR commented Feb 6, 2018

Ping @apapirovski

@apapirovski
Copy link
Member

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
Copy link
Member

BridgeAR commented Feb 6, 2018

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
Copy link
Member

apapirovski commented Feb 6, 2018

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 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
Copy link
Member

Landed in befe675 🎉

@BridgeAR BridgeAR closed this Feb 16, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 16, 2018
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>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
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>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
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>
@Leko Leko deleted the fix_flaky_net_econnrefused branch February 26, 2018 13:25
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
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>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
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>
MylesBorins pushed a commit that referenced this pull request Aug 7, 2018
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>
MylesBorins pushed a commit that referenced this pull request Aug 9, 2018
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>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2018
MylesBorins pushed a commit that referenced this pull request Aug 16, 2018
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>
lpinca added a commit to lpinca/node that referenced this pull request Mar 30, 2019
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
lpinca added a commit to lpinca/node that referenced this pull request Apr 27, 2019
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>
targos pushed a commit that referenced this pull request Apr 27, 2019
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flaky net-connect-handle-econnrefused
10 participants