Deflake JettyTransporterTest.testGet_HTTP3 (UDP port collision) - #2037
Merged
Conversation
gnodet
approved these changes
Aug 5, 2026
gnodet
left a comment
Contributor
There was a problem hiding this comment.
Clean, well-reasoned fix that correctly identifies the root cause: TCP and UDP port namespaces are independent, so an OS-assigned TCP port may collide with an already-occupied UDP port. The probe-and-retry approach finds a port free in both namespaces.
Observations (non-blocking):
- The TOCTOU race between the probe closing sockets and connectors binding is inherent to any port-probing approach, and the 20-iteration retry loop makes exhaustion statistically negligible (~10⁻⁴⁰ failure probability at 1% collision rate).
- The refactoring of
addHttp2Connectorto accept an optionalportparameter is backward-compatible — all existing no-arg callers pass-1(OS-assigned). - Confirmed this is the only place in the codebase that reuses a TCP port number for a UDP binding.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
|
@elharo Please assign appropriate label to PR according to the type of change. |
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.
Fixes #2036
Problem
JettyTransporterTest.testGet_HTTP3is flaky: binding the HTTP/3 (UDP) connector to the HTTPS TCP connector's port fails withjava.net.BindException: Address already in use.Root cause
The test bound both connectors to the same numeric port by reusing
getHttpsPort(), which is an OS-assigned TCP port. TCP and UDP port spaces are allocated independently, so the same-numbered UDP port may already be in use by another process. The collision is inherent to the "let the OS pick a TCP port, then force UDP onto the same number" pattern (verified by experiment: ~1% collision rate with a few hundred UDP sockets occupied).Fix
HttpServer.findFreeTcpAndUdpPort(), which probes for a port that is free for both TCP and UDP.HttpServer.addHttp2OnlyConnectorWithMutualTLS(int port)so the HTTPS (HTTP/2) connector can be bound to that exact port.testGet_HTTP3now reserves such a port and binds both connectors to it, keeping the intended "HTTP/2 and HTTP/3 on the same port" semantics.Verified:
JettyTransporterTest(89 tests),ApacheTransporterTest(93), andJdkTransporterTest(92) all pass; the HTTP/3 requests intestGet_HTTP3are served over HTTP/3.0.