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.

Sign up for GitHub

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

Jump to bottom

test: move test-net-connect-handle-econnrefused to sequential #27014

Closed
wants to merge 2 commits into from
Closed

test: move test-net-connect-handle-econnrefused to sequential #27014

wants to merge 2 commits into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Mar 30, 2019

First commit:

test: move test-net-connect-handle-econnrefused
    
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: https://github.com/nodejs/node/pull/18257#discussion_r162717096
Fixes: https://github.com/nodejs/node/issues/26907

Second commit:

test: refactor net-connect-handle-econnrefused
    
- Remove unneeded server
- Use `common.PORT`
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@lpinca
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
test: refactor net-connect-handle-econnrefused
325fd72 
- Remove unneeded server
- Use `common.PORT`
@nodejs-github-bot
Copy link
Collaborator

@lpinca build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/3108/pipeline

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 30, 2019
@mscdex
Copy link
Contributor

mscdex commented Mar 30, 2019

I don't see how this really changes anything? common.PORT is not any more special than a random port as far as the OS is concerned, any other process could start listening on it.

@lpinca
Copy link
Member Author

lpinca commented Mar 30, 2019 •
edited
Loading

@mscdex it is no longer run in parallel. First commit moves it to test/sequential. The assumption is that common.PORT is not taken of course. If that is not true a lot of tests in test/sequential will fails as well.

@mscdex
Copy link
Contributor

mscdex commented Mar 30, 2019

Instead of moving this test, why don't we just try to connect to port 0 on localhost? That should always fail with ECONNREFUSED without any potential problems.

@lpinca
Copy link
Member Author

lpinca commented Mar 30, 2019

Sounds good, will update in a bit.

@lpinca
Copy link
Member Author

lpinca commented Mar 30, 2019 •
edited
Loading

@mscdex it doesn't work on Windows. A different errno/code is returned.

@mscdex
Copy link
Contributor

mscdex commented Mar 30, 2019 •
edited
Loading

FWIW it looks like we could do this, but it's a little more involved for Windows. Instead of simply connecting to port 0 (which I think we could safely do for non-Windows) you can create a child process running the TCP server on a random port, suspend the process (doable via powershell), and then make connections to the server until you get ECONNREFUSED. It simulates the TCP backlog filling up and the OS rejecting new connections. I tried doing the same on Linux, but a stopped process there causes client connections to just hang and retry indefinitely it seems.

@BridgeAR
Copy link
Member

What's the status here?

@lpinca
Copy link
Member Author

lpinca commented Apr 16, 2019

Waiting for feedbacks/reviews. In my opinion @mscdex's latest suggestion is too complex to solve an issue with a test that has problem when run in parallel. We already have a dedicated set of tests running sequentially.

cc: @nodejs/testing

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 16, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@lpinca
Copy link
Member Author

lpinca commented Apr 26, 2019

@nodejs/collaborators any more opinions/suggestions?

lpinca added a commit to lpinca/node that referenced this pull request Apr 27, 2019
@lpinca
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>
lpinca added a commit to lpinca/node that referenced this pull request Apr 27, 2019
@lpinca
test: refactor net-connect-handle-econnrefused
66cf4b5 
- Remove unneeded server
- Use `common.PORT`

PR-URL: nodejs#27014
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@lpinca
Copy link
Member Author

lpinca commented Apr 27, 2019 •
edited
Loading

Landed in eca71e5...66cf4b5.

@lpinca lpinca closed this Apr 27, 2019
targos pushed a commit that referenced this pull request Apr 27, 2019
@lpinca @targos
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>
targos pushed a commit that referenced this pull request Apr 27, 2019
@lpinca @targos
test: refactor net-connect-handle-econnrefused
50732c1 
- Remove unneeded server
- Use `common.PORT`

PR-URL: #27014
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targos targos mentioned this pull request Apr 27, 2019
Merged
@lpinca lpinca deleted the gh-26907 branch April 27, 2019 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@benjamingr benjamingr benjamingr approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lpinca @nodejs-github-bot @mscdex @BridgeAR @Trott @benjamingr