FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix flaky net-connect-handle-econnrefused by Leko · Pull Request #18257 · nodejs/node · GitHub

/ node Public

fix flaky net-connect-handle-econnrefused - #18257

Closed
Leko wants to merge 2 commits into
nodejs:masterfrom
Leko:fix_flaky_net_econnrefused
ConversationCommits (2)ChecksFiles changed
Closed

fix flaky net-connect-handle-econnrefused#18257
Leko wants to merge 2 commits into
nodejs:masterfrom
Leko:fix_flaky_net_econnrefused

Conversation

Leko commented Jan 19, 2018

Copy link
Copy Markdown
Contributor

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 added the test Issues and PRs related to the tests. label Jan 19, 2018
cjihrig reviewed Jan 19, 2018
View reviewed changes
Comment thread test/parallel/test-net-connect-handle-econnrefused.js Outdated
assert.strictEqual('ECONNREFUSED', e.code);
}));
server.close();
server.close(() => {

cjihrig Jan 19, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Leko Jan 20, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

apapirovski Jan 20, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 approved these changes Jan 20, 2018
View reviewed changes

joyeecheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread test/parallel/test-net-connect-handle-econnrefused.js Outdated
assert.strictEqual('ECONNREFUSED', e.code);
}));
server.close();
server.close(() => {

joyeecheung Jan 20, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you wrap this in common.mustCall?

Leko Jan 20, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

targos commented Jan 21, 2018

Copy link
Copy Markdown
Member

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

lpinca commented Jan 21, 2018

Copy link
Copy Markdown
Member

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 commented Feb 1, 2018

Copy link
Copy Markdown
Member

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 commented Feb 6, 2018

Copy link
Copy Markdown
Member

Ping @apapirovski

apapirovski commented Feb 6, 2018

Copy link
Copy Markdown
Contributor

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 commented Feb 6, 2018

Copy link
Copy Markdown
Member

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 commented Feb 6, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 13, 2018

BridgeAR commented Feb 16, 2018

Copy link
Copy Markdown
Member

Landed in befe675 🎉

BridgeAR closed this Feb 16, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 16, 2018
test: try to connect after server was closed
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>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
test: try to connect after server was closed
e0096ed 
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 mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
test: try to connect after server was closed
8b5ca48 
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 deleted the fix_flaky_net_econnrefused branch February 26, 2018 13:25
apapirovski mentioned this pull request Apr 12, 2018
Closed
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
test: try to connect after server was closed
c4ec191 
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
test: try to connect after server was closed
7fbc6a4 
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
test: try to connect after server was closed
f13076f 
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
test: try to connect after server was closed
eb990a7 
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 mentioned this pull request Aug 16, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Aug 16, 2018
test: try to connect after server was closed
29009ae 
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
test: move test-net-connect-handle-econnrefused
c1c2b5c 
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
test: move test-net-connect-handle-econnrefused
f7afed1 
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
test: move test-net-connect-handle-econnrefused
e9021cc 
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cjihrig cjihrig left review comments
jasnell jasnell approved these changes
joyeecheung joyeecheung approved these changes
BridgeAR BridgeAR approved these changes
+2 more reviewers
apapirovski apapirovski approved these changes
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

flaky net-connect-handle-econnrefused

10 participants

Back | FazBrowse Home | New Git URL