-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Use HPACK spelling for HTTP/2 compression #2271
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,13 +106,16 @@ | |
| import static org.asynchttpclient.util.AuthenticatorUtils.perConnectionProxyAuthorizationHeader; | ||
| import static org.asynchttpclient.util.HttpConstants.Methods.CONNECT; | ||
| import static org.asynchttpclient.util.HttpConstants.Methods.GET; | ||
| import static org.asynchttpclient.util.HttpUtils.GZIP_DEFLATE; | ||
| import static org.asynchttpclient.util.HttpUtils.hostHeader; | ||
| import static org.asynchttpclient.util.MiscUtils.getCause; | ||
| import static org.asynchttpclient.util.ProxyUtils.getProxyServer; | ||
|
|
||
| public final class NettyRequestSender { | ||
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(NettyRequestSender.class); | ||
| private static final AsciiString HTTP2_HPACK_GZIP_DEFLATE = | ||
| new AsciiString(HttpHeaderValues.GZIP + ", " + HttpHeaderValues.DEFLATE); | ||
|
|
||
| private final AsyncHttpClientConfig config; | ||
| private final ChannelManager channelManager; | ||
|
|
@@ -918,6 +921,7 @@ private <T> void sendHttp2Frames(NettyResponseFuture<T> future, Http2StreamChann | |
| // Copy the HTTP/1.1 headers, dropping connection-specific names forbidden in HTTP/2 (RFC 7540 | ||
| // §8.1.2.2). iteratorCharSequence() avoids the per-name String the String-typed iterator forces; | ||
| // see isHttp2ExcludedHeader and toLowerCaseHeaderName for the skip-check and lowercasing rules. | ||
| boolean preferHpackAcceptEncoding = preferHpackAcceptEncoding(future.getCurrentRequest()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This reads the policy from future.getCurrentRequest() while the header values a few lines below come from future.getNettyRequest(). They happen to always be set together today in newNettyRequestAndResponseFuture, so this works, but it is two different reads off the same future that have to stay aligned. If you switch to the identity check on GZIP_DEFLATE mentioned below this whole method and this line can go away. |
||
| Iterator<Map.Entry<CharSequence, CharSequence>> it = httpRequest.headers().iteratorCharSequence(); | ||
| while (it.hasNext()) { | ||
| Map.Entry<CharSequence, CharSequence> entry = it.next(); | ||
|
|
@@ -933,7 +937,7 @@ private <T> void sendHttp2Frames(NettyResponseFuture<T> future, Http2StreamChann | |
| && !HttpHeaderValues.TRAILERS.contentEqualsIgnoreCase(value)) { | ||
| continue; | ||
| } | ||
| h2Headers.add(toLowerCaseHeaderName(name), value); | ||
| h2Headers.add(toLowerCaseHeaderName(name), http2HeaderValue(name, value, preferHpackAcceptEncoding)); | ||
| } | ||
|
|
||
| // Determine the body to send: an in-memory buffer (DefaultFullHttpRequest content or a | ||
|
|
@@ -971,6 +975,20 @@ private <T> void sendHttp2Frames(NettyResponseFuture<T> future, Http2StreamChann | |
| } | ||
| } | ||
|
|
||
| private boolean preferHpackAcceptEncoding(Request request) { | ||
| return config.isCompressionEnforced() | ||
| && !request.getHeaders().contains(HttpHeaderNames.ACCEPT_ENCODING); | ||
| } | ||
|
|
||
| private static CharSequence http2HeaderValue(CharSequence name, CharSequence value, boolean preferHpackAcceptEncoding) { | ||
| if (preferHpackAcceptEncoding | ||
| && HttpHeaderNames.ACCEPT_ENCODING.contentEqualsIgnoreCase(name) | ||
| && GZIP_DEFLATE.contentEquals(value)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since GZIP_DEFLATE is a singleton AsciiString and NettyRequestFactory stores that exact reference when it calls headers.set(ACCEPT_ENCODING, GZIP_DEFLATE), you can check value == GZIP_DEFLATE here instead of contentEquals. I checked and the reference does survive into the iterator, and a user typed string with the same text is never the same reference, so identity alone tells you whether this is the auto generated header without needing the separate preferHpackAcceptEncoding check at all. That would let you delete preferHpackAcceptEncoding and the getCurrentRequest call above, which removes a second read of state that has to stay in sync with what actually built the headers. |
||
| return HTTP2_HPACK_GZIP_DEFLATE; | ||
| } | ||
| return value; | ||
| } | ||
|
|
||
| /** | ||
| * Sends the body of an HTTP/2 request whose HEADERS were already written with {@code endStream=false} | ||
| * because it carried {@code Expect: 100-continue}. Invoked by {@code Continue100Interceptor} once the | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,6 +75,7 @@ | |
| import java.util.concurrent.atomic.AtomicReference; | ||
| import java.util.function.Consumer; | ||
|
|
||
| import static io.netty.handler.codec.http.HttpHeaderNames.ACCEPT_ENCODING; | ||
| import static io.netty.handler.codec.http.HttpHeaderNames.CONTENT_TYPE; | ||
| import static java.nio.charset.StandardCharsets.UTF_8; | ||
| import static java.util.concurrent.TimeUnit.SECONDS; | ||
|
|
@@ -1048,6 +1049,31 @@ public void mixedCaseHeaderIsLowercasedAndConnectionHeadersExcludedOverHttp2() t | |
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void generatedAcceptEncodingUsesHpackStaticValueOverHttp2() throws Exception { | ||
| try (AsyncHttpClient client = http2ClientWithConfig(builder -> builder.setCompressionEnforced(true))) { | ||
| Response response = client.prepareGet(httpsUrl("/echo")) | ||
| .execute() | ||
| .get(30, SECONDS); | ||
|
|
||
| assertEquals(200, response.getStatusCode()); | ||
| assertEquals("gzip, deflate", response.getHeader("X-accept-encoding")); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void userAcceptEncodingSpellingIsPreservedOverHttp2() throws Exception { | ||
| try (AsyncHttpClient client = http2ClientWithConfig(builder -> builder.setCompressionEnforced(true))) { | ||
| Response response = client.prepareGet(httpsUrl("/echo")) | ||
| .setHeader(ACCEPT_ENCODING, "gzip,deflate") | ||
| .execute() | ||
| .get(30, SECONDS); | ||
|
|
||
| assertEquals(200, response.getStatusCode()); | ||
| assertEquals("gzip,deflate", response.getHeader("X-accept-encoding")); | ||
| } | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two tests only cover HTTP/2. Since the whole point of the change is that HTTP/1 output stays as gzip,deflate with no space, it would be worth adding a small HTTP/1 test with compressionEnforced true asserting the header is still sent without the space, so that contract is pinned instead of just asserted in the PR description. |
||
|
|
||
| @Test | ||
| public void postWithHeadersAndFormParamsOverHttp2() throws Exception { | ||
| try (AsyncHttpClient client = http2Client()) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| /* | ||
| * Copyright (c) 2026 AsyncHttpClient Project. All rights reserved. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.asynchttpclient.netty.request; | ||
|
|
||
| import io.netty.buffer.ByteBuf; | ||
| import io.netty.buffer.Unpooled; | ||
| import io.netty.handler.codec.http2.DefaultHttp2Headers; | ||
| import io.netty.handler.codec.http2.DefaultHttp2HeadersEncoder; | ||
| import io.netty.handler.codec.http2.Http2Headers; | ||
| import io.netty.handler.codec.http2.Http2HeadersEncoder; | ||
| import io.netty.util.AsciiString; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| public class AcceptEncodingHpackTest { | ||
|
|
||
| private static final AsciiString ACCEPT_ENCODING = AsciiString.cached("accept-encoding"); | ||
|
|
||
| @Test | ||
| public void staticTableSpellingUsesOneByteIndexedRepresentation() throws Exception { | ||
| assertTrue(encodedLength("gzip,deflate") > 1); | ||
| assertEquals(1, encodedLength("gzip, deflate")); | ||
| } | ||
|
|
||
| private static int encodedLength(String value) throws Exception { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test builds headers by hand and drives Netty's encoder directly, it does not go through AHC's own request building or NettyRequestSender at all. It is a good proof that the two spellings differ in encoded size, but it is not proof that AHC actually produces the gzip, deflate spelling in production. That part is only covered by the two BasicHttp2Test cases. Might be worth a one line comment on the class saying that explicitly so nobody later assumes this test exercises the real code path. |
||
| Http2HeadersEncoder encoder = new DefaultHttp2HeadersEncoder(); | ||
| Http2Headers headers = new DefaultHttp2Headers().add(ACCEPT_ENCODING, value); | ||
| ByteBuf output = Unpooled.buffer(); | ||
| try { | ||
| encoder.encodeHeaders(3, headers, output); | ||
| return output.readableBytes(); | ||
| } finally { | ||
| output.release(); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider moving this next to HttpUtils.GZIP_DEFLATE in HttpUtils rather than keeping it here. They are really the same concept, the HPACK static table spelling of the same value, and keeping them in separate files makes it easier for someone to change one and forget the other later. A short comment noting this is RFC 7541 appendix A static table entry 16 would also help since that context is currently only in the benchmark javadoc, not on the field itself.