feat(listener): bind a TCP listener on port 0 and report what it got - #302
Merged
Merged
Conversation
Naming a free port before binding it leaves a window in which the port belongs to nobody, and a parallel suite walks into it: a second server's start() answers `Failed to acquire TCP listener for 127.0.0.1:58864 (bind)`. Port 0 removes the window instead of narrowing it — the kernel assigns at bind time — and HttpServer::getBoundListeners() is where the assignment becomes readable: one entry per configured listener, in configuration order, empty while the server is not running. Across threads such a listener is bound once into the shared set and every thread adopts a duplicate, SO_REUSEPORT or not; binding per thread would hand each a different port. The set now keys its entries on the listener's position in the configuration rather than on host and port, because two listeners asking for 0 look alike until the kernel answers, and it records the bound port beside the requested one — a pool parent builds no listen event of its own, so the set holds its only answer. HTTP/3 keeps requiring an explicit port: it binds through the UDP path, which reports no local address, so 0 is refused there rather than answered with a guess. `core/032-config-validation` asserted that 0 throws on the three TCP adders. That expectation is the behaviour this changes; it now expects acceptance for TCP, a throw for H3, and covers -1 for the lower bound. Evidence: `core/078-listener-port-zero` covers the single server and a two-worker pool, both serving a request on the assigned port, and passes 5 of 5 runs; the Windows suite reads 380 tests, 169 passed, 0 warned, 0 failed, 211 skipped.
Contributor
CoverageTotal lines: 82.98% → 83.13% (+0.15 pp)
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A caller that names a free port before binding it leaves a window in which the port belongs to nobody. The phpt suite picks ports that way, and under
-j4it loses the race:core/015starts a second server after the first has run its whole lifecycle, andstart()answersFailed to acquire TCP listener for 127.0.0.1:58864 (bind). Over thirty tests take ports from that helper, so narrowing the window only makes the failure rarer.Port 0 removes it.
addListener(),addHttp1Listener()andaddHttp2Listener()accept 0, the kernel assigns at bind time, andHttpServer::getBoundListeners()reports what the server holds — one entry per configured listener, in configuration order, empty while the server is not running. A TCP entry carrieshost,portandtls; a UNIX entry carriespathand noportkey at all, so an absent answer cannot be mistaken for a number.The answer lives on the server rather than in the config: the point of port 0 is that one config can be handed to a second server, and a config that recorded the first server's port would send the second one to bind it — the very bug this fixes. A restarted server rebinds and reports the new port for the same reason.
Across threads. Such a listener is bound once into the shared set and every thread adopts a duplicate, SO_REUSEPORT or not — a per-thread bind would give each thread a different port. The set keys its entries on the listener's position in the configuration instead of on host and port, since two listeners asking for 0 are indistinguishable until the kernel answers, and it records the bound port beside the requested one: a pool parent builds no listen event of its own, so the set holds its only answer. Two listeners naming the same host:port still share one socket, as before.
HTTP/3 keeps requiring an explicit port. It binds through
ZEND_ASYNC_UDP_BIND, which reports no local address, so 0 is refused there rather than answered with the number that was asked for.An old expectation changed.
core/032-config-validationasserted that port 0 throws on the three TCP adders — that is the behaviour this changes. It now expects acceptance for TCP and a throw for H3, and a-1case keeps the lower bound covered.Evidence: the extension builds clean on Windows (MSVC,
/W4, no new warnings);core/078-listener-port-zerocovers a single server and a two-worker pool, each serving a request on the assigned port, and passes 5 of 5 runs; the Windows suite reads 380 tests, 169 passed, 0 warned, 0 failed, 211 skipped.Open, and named rather than hidden: the arginfo header was hand-edited because
gen_stub.phpneeds the tokenizer extension, which this build lacks — a reviewer regenerating it should get a byte-identical file.