-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge pull request #5372 from bjornhusberg/master
### Motivation The Http2 server-implementation handles content encoding differently compared to the Http1.x server-implementation, which leads to issues. Http1.x: The HttpContentCompressor determines the encoding based on the Accept-Encoding header right before writing the headers. If the request handler has already set a Content-Encoding header there will be no further encoding and if the Content-Encoding header is set to "identity" the header is removed by HttpChunkContentCompressor and the response is returned as-is. Http2: The encoding is determined already when setting up the stream in Http2ServerConnection and is then passed down to Http2ServerRequest and Http2ServerResponse where it is set using the Content-Encoding-header. The header content is later picked up by the CompressorHttp2ConnectionEncoder which compresses the response accordingly. While the Http1.x-implementation works fine the Http2-implementation leads to two problems: 1. If the response is already encoded by the request handler and the Content-Encoding header has been set accordingly, the CompressorHttp2ConnectionEncoder picks up the encoding and encodes the response again, resulting in garbage returned to the client. 2. If the Content-Encoding is explicitly set to "identity", the response is not compressed by the CompressorHttp2ConnectionEncoder but the Content-Encoding holding "identity" leaks out to the client. This is unnecessary and leads to confusion for some clients. ### Changes Removes the setting of the Content-Encoding header in Http2ServerResponse. - _Change motivation:_ Setting the header in one place and performing the compression in another makes it difficult to handle scenarios where the header is set by other code. For instance a request handler. Setting the encoding in the HttpServerResponse is also not the way it's done in the Http1-implementation. - _Actual changes:_ Removed the setting of the header from Http2ServerResponse. After that, the contentEncoding variable could be removed from the Http2ServerResponse, the Http2ServerRequest and the Push class and no longer needed to be determined in the Http2ServerConnection.initStream or Http2ServerConnection.doSendPush. Extended the CompressorHttp2ConnectionEncoder with a new VertxCompressorHttp2ConnectionEncoder. - _Change motivation:_ The new encoder determines the content-encoding right before writing the headers, similar to how it's done in HttpContentCompressor. This makes it possible to handle the case where the payload has already been encoded by a request handler or explicitly disabled using Content-Encoding: identity. - _Actual changes:_ Added a VertxCompressorHttp2ConnectionEncoder that determines if the payload should be compressed just before writing the headers. If content-encoding has already been set or could not be determined the compressor encoder is skipped. If the Content-Encoding header is set to "identity" it is removed. Adds two new tests in Http2ServerTest that verifies the issues described in the GitHub issue. ### Outcome Fixes #5371
- Loading branch information
Showing
8 changed files
with
298 additions
and
26 deletions.
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
181 changes: 181 additions & 0 deletions
181
vertx-core/src/main/java/io/vertx/core/http/impl/VertxCompressorHttp2ConnectionEncoder.java
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,181 @@ | ||
| /* | ||
| * Copyright (c) 2011-2024 Contributors to the Eclipse Foundation | ||
| * | ||
| * This program and the accompanying materials are made available under the | ||
| * terms of the Eclipse Public License 2.0 which is available at | ||
| * http://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 | ||
| * which is available at https://www.apache.org/licenses/LICENSE-2.0. | ||
| * | ||
| * SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 | ||
| */ | ||
| package io.vertx.core.http.impl; | ||
|
|
||
| import io.netty.buffer.ByteBuf; | ||
| import io.netty.channel.ChannelFuture; | ||
| import io.netty.channel.ChannelHandlerContext; | ||
| import io.netty.channel.ChannelPromise; | ||
| import io.netty.handler.codec.compression.CompressionOptions; | ||
| import io.netty.handler.codec.http2.CompressorHttp2ConnectionEncoder; | ||
| import io.netty.handler.codec.http2.Http2Connection; | ||
| import io.netty.handler.codec.http2.Http2ConnectionEncoder; | ||
| import io.netty.handler.codec.http2.Http2Exception; | ||
| import io.netty.handler.codec.http2.Http2Flags; | ||
| import io.netty.handler.codec.http2.Http2FrameWriter; | ||
| import io.netty.handler.codec.http2.Http2Headers; | ||
| import io.netty.handler.codec.http2.Http2LifecycleManager; | ||
| import io.netty.handler.codec.http2.Http2RemoteFlowController; | ||
| import io.netty.handler.codec.http2.Http2Settings; | ||
| import io.netty.handler.codec.http2.Http2SettingsReceivedConsumer; | ||
| import java.util.function.Function; | ||
|
|
||
| import static io.vertx.core.http.HttpHeaders.CONTENT_ENCODING; | ||
| import static io.vertx.core.http.HttpHeaders.IDENTITY; | ||
|
|
||
| public class VertxCompressorHttp2ConnectionEncoder implements Http2FrameWriter, Http2ConnectionEncoder, Http2SettingsReceivedConsumer { | ||
|
|
||
| private Http2ConnectionEncoder delegate; | ||
| private final Http2ConnectionEncoder plainEncoder; | ||
|
|
||
| public VertxCompressorHttp2ConnectionEncoder(Http2ConnectionEncoder plainEncoder, CompressionOptions[] compressionOptions) { | ||
| this.delegate = new CompressorHttp2ConnectionEncoder(plainEncoder, compressionOptions); | ||
| this.plainEncoder = plainEncoder; | ||
| } | ||
|
|
||
| private void beforeWritingHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers responseHeaders) { | ||
| String contentEncodingToApply = determineContentEncodingToApply(ctx, streamId, responseHeaders); | ||
| if (contentEncodingToApply == null || contentEncodingToApply.equalsIgnoreCase(IDENTITY.toString())) { | ||
| if (responseHeaders.contains(CONTENT_ENCODING, IDENTITY)) { | ||
| responseHeaders.remove(CONTENT_ENCODING); | ||
| } | ||
| delegate = plainEncoder; | ||
| } else { | ||
| responseHeaders.set(CONTENT_ENCODING, contentEncodingToApply); | ||
| } | ||
| } | ||
|
|
||
| private String determineContentEncodingToApply(ChannelHandlerContext ctx, int streamId, Http2Headers responseHeaders) { | ||
| if (responseHeaders.contains(CONTENT_ENCODING)) { | ||
| return null; | ||
| } | ||
| return ifType(ctx.handler(), VertxHttp2ConnectionHandler.class, connectionHandler -> | ||
| ifType(connectionHandler.connectFuture().getNow(), Http2ServerConnection.class, connection -> | ||
| ifType(connection.stream(streamId), Http2ServerStream.class, stream -> | ||
| stream.headers == null ? null : connection.determineContentEncoding(stream.headers)))); | ||
| } | ||
|
|
||
| private <T, R> R ifType(Object obj, Class<T> type, Function<T, R> then) { | ||
| return obj != null && type.isAssignableFrom(obj.getClass()) ? then.apply(type.cast(obj)) : null; | ||
| } | ||
|
|
||
| @Override | ||
| public ChannelFuture writeHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers headers, int padding, boolean endStream, ChannelPromise promise) { | ||
| beforeWritingHeaders(ctx, streamId, headers); | ||
| return delegate.writeHeaders(ctx, streamId, headers, padding, endStream, promise); | ||
| } | ||
|
|
||
| @Override | ||
| public ChannelFuture writeHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers headers, int streamDependency, short weight, boolean exclusive, int padding, boolean endStream, ChannelPromise promise) { | ||
| beforeWritingHeaders(ctx, streamId, headers); | ||
| return delegate.writeHeaders(ctx, streamId, headers, streamDependency, weight, exclusive, padding, endStream, promise); | ||
| } | ||
|
|
||
| @Override | ||
| public void lifecycleManager(Http2LifecycleManager http2LifecycleManager) { | ||
| delegate.lifecycleManager(http2LifecycleManager); | ||
| } | ||
|
|
||
| @Override | ||
| public Http2Connection connection() { | ||
| return delegate.connection(); | ||
| } | ||
|
|
||
| @Override | ||
| public Http2RemoteFlowController flowController() { | ||
| return delegate.flowController(); | ||
| } | ||
|
|
||
| @Override | ||
| public Http2FrameWriter frameWriter() { | ||
| return delegate.frameWriter(); | ||
| } | ||
|
|
||
| @Override | ||
| public Http2Settings pollSentSettings() { | ||
| return delegate.pollSentSettings(); | ||
| } | ||
|
|
||
| @Override | ||
| public void remoteSettings(Http2Settings http2Settings) throws Http2Exception { | ||
| delegate.remoteSettings(http2Settings); | ||
| } | ||
|
|
||
| @Override | ||
| public ChannelFuture writePriority(ChannelHandlerContext ctx, int streamId, int streamDependency, short weight, boolean exclusive, ChannelPromise promise) { | ||
| return delegate.writePriority(ctx, streamId, streamDependency, weight, exclusive, promise); | ||
| } | ||
|
|
||
| @Override | ||
| public ChannelFuture writeRstStream(ChannelHandlerContext ctx, int streamId, long errorCode, ChannelPromise promise) { | ||
| return delegate.writeRstStream(ctx, streamId, errorCode, promise); | ||
| } | ||
|
|
||
| @Override | ||
| public ChannelFuture writeSettings(ChannelHandlerContext ctx, Http2Settings settings, ChannelPromise promise) { | ||
| return delegate.writeSettings(ctx, settings, promise); | ||
| } | ||
|
|
||
| @Override | ||
| public ChannelFuture writeSettingsAck(ChannelHandlerContext ctx, ChannelPromise promise) { | ||
| return delegate.writeSettingsAck(ctx, promise); | ||
| } | ||
|
|
||
| @Override | ||
| public ChannelFuture writePing(ChannelHandlerContext ctx, boolean ack, long data, ChannelPromise promise) { | ||
| return delegate.writePing(ctx, ack, data, promise); | ||
| } | ||
|
|
||
| @Override | ||
| public ChannelFuture writePushPromise(ChannelHandlerContext ctx, int streamId, int promisedStreamId, Http2Headers headers, int padding, ChannelPromise promise) { | ||
| return delegate.writePushPromise(ctx, streamId, promisedStreamId, headers, padding, promise); | ||
| } | ||
|
|
||
| @Override | ||
| public ChannelFuture writeGoAway(ChannelHandlerContext ctx, int lastStreamId, long errorCode, ByteBuf debugData, ChannelPromise promise) { | ||
| return delegate.writeGoAway(ctx, lastStreamId, errorCode, debugData, promise); | ||
| } | ||
|
|
||
| @Override | ||
| public ChannelFuture writeWindowUpdate(ChannelHandlerContext ctx, int streamId, int windowSizeIncrement, ChannelPromise promise) { | ||
| return delegate.writeWindowUpdate(ctx, streamId, windowSizeIncrement, promise); | ||
| } | ||
|
|
||
| @Override | ||
| public ChannelFuture writeFrame(ChannelHandlerContext ctx, byte frameType, int streamId, Http2Flags flags, ByteBuf payload, ChannelPromise promise) { | ||
| return delegate.writeFrame(ctx, frameType, streamId, flags, payload, promise); | ||
| } | ||
|
|
||
| @Override | ||
| public Configuration configuration() { | ||
| return delegate.configuration(); | ||
| } | ||
|
|
||
| @Override | ||
| public void close() { | ||
| delegate.close(); | ||
| } | ||
|
|
||
| @Override | ||
| public ChannelFuture writeData(ChannelHandlerContext ctx, int streamId, ByteBuf data, int padding, boolean endStream, ChannelPromise promise) { | ||
| return delegate.writeData(ctx, streamId, data, padding, endStream, promise); | ||
| } | ||
|
|
||
| @Override | ||
| public void consumeReceivedSettings(Http2Settings settings) { | ||
| if (delegate instanceof Http2SettingsReceivedConsumer) { | ||
| ((Http2SettingsReceivedConsumer) delegate).consumeReceivedSettings(settings); | ||
| } else { | ||
| throw new IllegalStateException("delegate " + delegate + " is not an instance of " + | ||
| Http2SettingsReceivedConsumer.class); | ||
| } | ||
| } | ||
| } |
This file contains 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
Oops, something went wrong.