Skip to content

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 added 2 commits March 30, 2019 15:09
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
- Remove unneeded server - Use `common.PORT`
@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

@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

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

@mscdex
Copy link
Contributor

mscdex commented Mar 30, 2019

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

Landed in eca71e5...66cf4b5.

@lpinca lpinca closed this Apr 27, 2019
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>
targos pushed a commit that referenced this pull request Apr 27, 2019
- 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
@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

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.

6 participants