-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
[native] Replace fixed worker port with ephemeral ports #22748
base: master
Are you sure you want to change the base?
Conversation
Just want to leave my $.02 here -- We had similar problems at my old job and we had attempted this type of solution to grab free worker ports. Ultimately it ended up being more reliable to pick a fixed port number that is usually not used by the OS for our E2E integration tests. This type of port selection didn't work because after releasing the socket back to the OS, we found a race condition occurred quite often where we would get assigned the same port back to back or in close succession before the port is actually allocated to the new server's socket. I think in this case, since we don't launch workers in parallel, we probably won't run into this situation as often, but I do have a PR which parallelizes the launching that would probably cause issues (#22212). I think a better solution would let the worker bind to port 0, and then query the process internally for its assigned port once the socket is returned by the OS to the worker. |
@ZacBlanco Thank you for your comment. I also thought of the prestissimo side. We don't need to define a fixed port in the config. The worker will tell the coordinator how to reach it during the announcement. But not sure what would be needed for the HttpServer - we pass in the http/https config. I would need to look into it a bit more. |
92c4482
to
751b140
Compare
751b140
to
ca3fa13
Compare
Previously the listener ports for the native works in the E2E tests was hard coded to be 1234 + worker count. The change looks in the OS for an available ephemeral port and uses this value when spawning the native workers. The native worker must then defer some configuration until the port selection by the OS is known.
ca3fa13
to
3ea39ef
Compare
@aditi-pandit FYI. I was thinking to add something to the C++ documentation about the |
@czentgr : Didn't entirely follow the reference in the question ? What do you want to document about http-server.port |
Previously the listener ports for the native works in the E2E tests was hard coded to be 1234 + worker count.
The change looks in the OS for an available ephemeral port and uses this value when spawning the native workers.
Description
Motivation and Context
On my Mac I encountered problems running the E2E native tests. The worker was up and running and listened on the port. Yet for some reason the HTTP request timed out. The connection was set up but there was no response.
Connection could occur but each request was eaten. Changing the port to a different port resolved the issue.
and in the logs
continuously until the test case fails.
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.