Support HTTP/2 after proxy CONNECT#2250
Conversation
536b173 to
e14569c
Compare
hyperxpro
left a comment
hyperxpro
left a comment
There was a problem hiding this comment.
Not in this PR's diff, so leaving it here: this change makes HTTP/2 after CONNECT work, which makes the workaround in client/src/test/java/org/asynchttpclient/proxy/HttpsProxyTestcontainersIntegrationTest.java stale and its comment misleading. Could you remove .setHttp2Enabled(false) and the // HTTP/2 ALPN upgrade after proxy CONNECT tunnel is not yet supported comment in both testHttpProxyToHttpsTarget (lines 149-150) and testHttpsProxyToHttpsTarget (lines 172-173)? That lets those external tests exercise the new HTTP/2-through-proxy path instead of forcing HTTP/1.1, and the comment stops claiming something that is no longer true.
| return; | ||
| } | ||
|
|
||
| whenHandshaked.addListener(f -> { |
There was a problem hiding this comment.
This upgrade has to run before the sendNextRequest listener that NettyRequestSender.drainChannelAndExecuteNextRequest adds to the same whenHandshaked future, otherwise writeRequest would see the pipeline still on HTTP/1.1 and send HTTP/1.1. It works today because this listener is registered first (during CONNECT handling) and the drain listener is registered later, when the CONNECT last-content is read, so Netty fires them in that order. That is a real correctness invariant, but it lives across two files and is easy to break by accident. Could we either add a short comment here spelling it out, or move the ALPN check and upgrade into the same listener that calls sendNextRequest so the ordering is guaranteed locally?
There was a problem hiding this comment.
Good catch. I moved the ALPN check and pipeline upgrade into the same handshake listener that sends the next request, so the ordering is local now.
| return updatePipelineForHttpTunneling(pipeline, requestUri, null); | ||
| } | ||
|
|
||
| public Future<Channel> updatePipelineForHttpTunneling(ChannelPipeline pipeline, Uri requestUri, Object partitionKey) { |
There was a problem hiding this comment.
Small design note: these methods were pure pipeline setup, and now they also schedule a post-handshake side effect. Passing partitionKey in just so it can be handed to registerHttp2Connection inside the listener works, but it does blur the responsibility a bit. Keeping the overloads for binary compatibility is a good call though.
There was a problem hiding this comment.
Agreed. I removed the new overloads and kept the pipeline setup methods focused on setup. The post-handshake upgrade now happens explicitly from NettyRequestSender, which already has the partition key.
| ChannelPipeline pipeline, | ||
| Uri requestUri, | ||
| Object partitionKey) { | ||
| if (!config.isHttp2Enabled() || requestUri.isWebSocket()) { |
There was a problem hiding this comment.
Minor: the config.isHttp2Enabled() half is belt and suspenders, since with HTTP/2 disabled the SSL engine never advertises h2 and applicationProtocol() will not equal h2. Harmless to keep, just noting it is not strictly required. The WebSocket guard is worth keeping.
There was a problem hiding this comment.
Yep, removed the redundant config check. We now rely on the negotiated ALPN result, with the WebSocket guard still in NettyRequestSender.
| if (f.isSuccess() && ApplicationProtocolNames.HTTP_2.equals(sslHandler.applicationProtocol())) { | ||
| upgradePipelineToHttp2(pipeline); | ||
| if (partitionKey != null) { | ||
| registerHttp2Connection(partitionKey, pipeline.channel()); |
There was a problem hiding this comment.
future.getPartitionKey() is effectively never null here, so the partitionKey != null guard is defensive only. Fine to keep, just flagging that if it ever were null the tunnel would upgrade but never get registered for reuse, and that would be silent.
There was a problem hiding this comment.
Removed the null guard. Registration now always uses future.getPartitionKey(), so a bad null cannot silently leave an upgraded connection unregistered.
| } | ||
|
|
||
| private void assertHttpsRequestsThroughProxyNegotiateAndReuseHttp2AfterConnect(boolean secureProxy) throws Exception { | ||
| Server proxy = new Server(); |
There was a problem hiding this comment.
proxy.start() is outside the try/finally. Nothing between here and the try throws today, so it is fine, but moving the start inside the try (or the Server proxy creation into a try-with-resources style teardown) would make the cleanup airtight if setup ever grows.
There was a problem hiding this comment.
Done -proxy.start() is now inside the try, so proxy.stop() still runs from finally if setup fails.
|
Done - removed |
|
Hi @hyperxpro, just a gentle reminder when you get a chance. I've addressed the review comments and would appreciate a re-review. Thanks! |
2 participants
Motivation
When an HTTPS request uses an HTTP proxy, AHC establishes a
CONNECTtunnel and then performs TLS with the target. If ALPN selectsh2, the tunnel path currently leaves the HTTP/1.1 codec in place. The target then receives HTTP/1.1 bytes on a connection that negotiated HTTP/2 and closes it.Modification
h2, upgrade the existing channel pipeline to HTTP/2 and register the physical tunnel connection with its existing pool partition key.CONNECTproxies with a TLS HTTP/2 target.Result
HTTPS requests through HTTP and HTTPS
CONNECTproxies now send HTTP/2 frames when the target negotiatesh2. The regression tests verify successful HTTP/2 responses and reuse of a single physical target connection.Testing
./mvnw -pl client -Dtest=BasicHttp2Test,HttpsProxyTest test— 60 tests passed.verify; the local run then stopped at artifact signing becausegpgis not installed../mvnw -pl client -DskipTests -Dgpg.skip=true verify— packaging and API compatibility checks passed.Fixes #2241