test: move test-net-connect-handle-econnrefused to sequential - #27014
test: move test-net-connect-handle-econnrefused to sequential#27014lpinca wants to merge 2 commits into
Conversation
Sorry, something went wrong.
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`
|
@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 |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Sounds good, will update in a bit. |
Sorry, something went wrong.
|
@mscdex it doesn't work on Windows. A different errno/code is returned. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
What's the status here? |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
Sorry, something went wrong.
|
@nodejs/collaborators any more opinions/suggestions? |
Sorry, something went wrong.
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>
- Remove unneeded server - Use `common.PORT` PR-URL: nodejs#27014 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
|
Landed in eca71e5...66cf4b5. |
Sorry, something went wrong.
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>
- Remove unneeded server - Use `common.PORT` PR-URL: #27014 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
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/26907Second commit:
test: refactor net-connect-handle-econnrefused - Remove unneeded server - Use `common.PORT`Checklist