From 7702bb5d786ced0a3713e52105b8e94a4920a44d Mon Sep 17 00:00:00 2001 From: graemerocher Date: Thu, 13 Jan 2022 12:48:51 +0100 Subject: [PATCH 1/4] Fix HTTP 304 responses for files don't associate the correct stream ID --- .../http/server/netty/NettyHttpRequest.java | 19 +++++++++++++++++++ .../netty/types/files/FileTypeHandler.java | 4 ++++ ...ttySystemFileCustomizableResponseType.java | 12 +++--------- 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java index 662aa22df0e..cf8c5bdbbf0 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java @@ -54,9 +54,11 @@ import io.netty.handler.codec.http.DefaultFullHttpRequest; import io.netty.handler.codec.http.DefaultHttpHeaders; import io.netty.handler.codec.http.DefaultHttpRequest; +import io.netty.handler.codec.http.DefaultHttpResponse; import io.netty.handler.codec.http.DefaultLastHttpContent; import io.netty.handler.codec.http.EmptyHttpHeaders; import io.netty.handler.codec.http.HttpHeaderNames; +import io.netty.handler.codec.http.HttpResponse; import io.netty.handler.codec.http.QueryStringDecoder; import io.netty.handler.codec.http.cookie.ClientCookieEncoder; import io.netty.handler.codec.http.multipart.AbstractHttpData; @@ -176,6 +178,23 @@ public NettyHttpRequest(io.netty.handler.codec.http.HttpRequest nettyRequest, this.body = SupplierUtil.memoizedNonEmpty(() -> Optional.ofNullable((T) buildBody())); } + /** + * Prepares a response based on this HTTP/2 request if HTTP/2 is enabled + * @param finalResponse The response to prepare, never {@code null} + */ + @Internal + public void prepareHttp2ResponseIfNecessary(@NonNull HttpResponse finalResponse) { + final io.micronaut.http.HttpVersion httpVersion = getHttpVersion(); + final boolean isHttp2 = httpVersion == io.micronaut.http.HttpVersion.HTTP_2_0; + if (isHttp2) { + final io.netty.handler.codec.http.HttpHeaders nativeHeaders = nettyRequest.headers(); + final String streamId = nativeHeaders.get(STREAM_ID); + if (streamId != null) { + finalResponse.headers().set(STREAM_ID, streamId); + } + } + } + @Override public MutableHttpRequest mutate() { return new NettyMutableHttpRequest(); diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/types/files/FileTypeHandler.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/types/files/FileTypeHandler.java index c5587ba9fdd..eeb1dd82aa2 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/types/files/FileTypeHandler.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/types/files/FileTypeHandler.java @@ -22,6 +22,7 @@ import io.micronaut.http.MutableHttpHeaders; import io.micronaut.http.MutableHttpResponse; import io.micronaut.http.netty.NettyMutableHttpResponse; +import io.micronaut.http.server.netty.NettyHttpRequest; import io.micronaut.http.server.netty.configuration.NettyHttpServerConfiguration; import io.micronaut.http.server.netty.types.NettyCustomizableResponseTypeHandler; import io.micronaut.http.server.netty.types.NettyFileCustomizableResponseType; @@ -87,6 +88,9 @@ public void handle(Object obj, HttpRequest request, MutableHttpResponse re long fileLastModifiedSeconds = lastModified / 1000; if (ifModifiedSinceDateSeconds == fileLastModifiedSeconds) { FullHttpResponse nettyResponse = notModified(response); + if (request instanceof NettyHttpRequest) { + ((NettyHttpRequest) request).prepareHttp2ResponseIfNecessary(nettyResponse); + } context.writeAndFlush(nettyResponse); return; } diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/types/files/NettySystemFileCustomizableResponseType.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/types/files/NettySystemFileCustomizableResponseType.java index cfe3aba5ef1..417b9b02524 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/types/files/NettySystemFileCustomizableResponseType.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/types/files/NettySystemFileCustomizableResponseType.java @@ -19,7 +19,6 @@ import io.micronaut.http.HttpRequest; import io.micronaut.http.MediaType; import io.micronaut.http.MutableHttpResponse; -import io.micronaut.http.netty.AbstractNettyHttpRequest; import io.micronaut.http.netty.NettyMutableHttpResponse; import io.micronaut.http.server.netty.NettyHttpRequest; import io.micronaut.http.server.netty.SmartHttpContentCompressor; @@ -119,14 +118,8 @@ public void write(HttpRequest request, MutableHttpResponse response, Chann // Write the request data final DefaultHttpResponse finalResponse = new DefaultHttpResponse(nettyResponse.getNettyHttpVersion(), nettyResponse.getNettyHttpStatus(), nettyResponse.getNettyHeaders()); - final io.micronaut.http.HttpVersion httpVersion = request.getHttpVersion(); - final boolean isHttp2 = httpVersion == io.micronaut.http.HttpVersion.HTTP_2_0; - if (isHttp2 && request instanceof NettyHttpRequest) { - final io.netty.handler.codec.http.HttpHeaders nativeHeaders = ((NettyHttpRequest) request).getNativeRequest().headers(); - final String streamId = nativeHeaders.get(AbstractNettyHttpRequest.STREAM_ID); - if (streamId != null) { - finalResponse.headers().set(AbstractNettyHttpRequest.STREAM_ID, streamId); - } + if (request instanceof NettyHttpRequest) { + ((NettyHttpRequest) request).prepareHttp2ResponseIfNecessary(finalResponse); } context.write(finalResponse, context.voidPromise()); @@ -159,4 +152,5 @@ public void write(HttpRequest request, MutableHttpResponse response, Chann throw new IllegalArgumentException("Unsupported response type. Not a Netty response: " + response); } } + } From 7e6e1af5e844f2378d4a24efdd1a0365d1a86321 Mon Sep 17 00:00:00 2001 From: graemerocher Date: Thu, 13 Jan 2022 12:51:17 +0100 Subject: [PATCH 2/4] fix checkstyle / make final --- .../io/micronaut/http/server/netty/NettyHttpRequest.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java index cf8c5bdbbf0..539024201f9 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java @@ -179,11 +179,12 @@ public NettyHttpRequest(io.netty.handler.codec.http.HttpRequest nettyRequest, } /** - * Prepares a response based on this HTTP/2 request if HTTP/2 is enabled + * Prepares a response based on this HTTP/2 request if HTTP/2 is enabled. + * * @param finalResponse The response to prepare, never {@code null} */ @Internal - public void prepareHttp2ResponseIfNecessary(@NonNull HttpResponse finalResponse) { + public final void prepareHttp2ResponseIfNecessary(@NonNull HttpResponse finalResponse) { final io.micronaut.http.HttpVersion httpVersion = getHttpVersion(); final boolean isHttp2 = httpVersion == io.micronaut.http.HttpVersion.HTTP_2_0; if (isHttp2) { From 0f7ad7e1c8dccaec390e8b5af0c3029ef77d73a6 Mon Sep 17 00:00:00 2001 From: graemerocher Date: Thu, 13 Jan 2022 13:00:54 +0100 Subject: [PATCH 3/4] checkstyle --- .../java/io/micronaut/http/server/netty/NettyHttpRequest.java | 3 +-- .../io/micronaut/http/server/netty/RoutingInBoundHandler.java | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java index 539024201f9..3afa5a66264 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java @@ -54,7 +54,6 @@ import io.netty.handler.codec.http.DefaultFullHttpRequest; import io.netty.handler.codec.http.DefaultHttpHeaders; import io.netty.handler.codec.http.DefaultHttpRequest; -import io.netty.handler.codec.http.DefaultHttpResponse; import io.netty.handler.codec.http.DefaultLastHttpContent; import io.netty.handler.codec.http.EmptyHttpHeaders; import io.netty.handler.codec.http.HttpHeaderNames; @@ -180,7 +179,7 @@ public NettyHttpRequest(io.netty.handler.codec.http.HttpRequest nettyRequest, /** * Prepares a response based on this HTTP/2 request if HTTP/2 is enabled. - * + * * @param finalResponse The response to prepare, never {@code null} */ @Internal diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/RoutingInBoundHandler.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/RoutingInBoundHandler.java index 700ef406237..ea806c732b7 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/RoutingInBoundHandler.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/RoutingInBoundHandler.java @@ -144,8 +144,8 @@ class RoutingInBoundHandler extends SimpleChannelInboundHandler> { private static final Logger LOG = LoggerFactory.getLogger(RoutingInBoundHandler.class); - /** - * Also present in {@link RouteExecutor} + /* + * Also present in {@link RouteExecutor}. */ private static final Pattern IGNORABLE_ERROR_MESSAGE = Pattern.compile( "^.*(?:connection.*(?:reset|closed|abort|broken)|broken.*pipe).*$", Pattern.CASE_INSENSITIVE); From fb0123d3d6293161e58b3b9e74cad3db42e200c0 Mon Sep 17 00:00:00 2001 From: yawkat Date: Thu, 13 Jan 2022 14:17:22 +0100 Subject: [PATCH 4/4] - test case - use common method in NettyStreamedCustomizableResponseType too --- ...NettyStreamedCustomizableResponseType.java | 8 +- .../Http2StaticResourceCacheSpec.groovy | 125 ++++++++++++++++++ 2 files changed, 127 insertions(+), 6 deletions(-) create mode 100644 http-server-netty/src/test/groovy/io/micronaut/http/server/netty/resources/Http2StaticResourceCacheSpec.groovy diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/types/stream/NettyStreamedCustomizableResponseType.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/types/stream/NettyStreamedCustomizableResponseType.java index 949e4600560..3e4a34c4994 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/types/stream/NettyStreamedCustomizableResponseType.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/types/stream/NettyStreamedCustomizableResponseType.java @@ -58,12 +58,8 @@ default void write(HttpRequest request, MutableHttpResponse response, Chan final DefaultHttpResponse finalResponse = new DefaultHttpResponse(nettyResponse.getNettyHttpVersion(), nettyResponse.getNettyHttpStatus(), nettyResponse.getNettyHeaders()); final io.micronaut.http.HttpVersion httpVersion = request.getHttpVersion(); final boolean isHttp2 = httpVersion == io.micronaut.http.HttpVersion.HTTP_2_0; - if (isHttp2 && request instanceof NettyHttpRequest) { - final io.netty.handler.codec.http.HttpHeaders nativeHeaders = ((NettyHttpRequest) request).getNativeRequest().headers(); - final String streamId = nativeHeaders.get(AbstractNettyHttpRequest.STREAM_ID); - if (streamId != null) { - finalResponse.headers().set(AbstractNettyHttpRequest.STREAM_ID, streamId); - } + if (request instanceof NettyHttpRequest) { + ((NettyHttpRequest) request).prepareHttp2ResponseIfNecessary(finalResponse); } InputStream inputStream = getInputStream(); // can be null if the stream was closed diff --git a/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/resources/Http2StaticResourceCacheSpec.groovy b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/resources/Http2StaticResourceCacheSpec.groovy new file mode 100644 index 00000000000..797b6dc260f --- /dev/null +++ b/http-server-netty/src/test/groovy/io/micronaut/http/server/netty/resources/Http2StaticResourceCacheSpec.groovy @@ -0,0 +1,125 @@ +package io.micronaut.http.server.netty.resources + +import io.micronaut.context.ApplicationContext +import io.micronaut.http.HttpHeaders +import io.micronaut.http.netty.AbstractNettyHttpRequest +import io.micronaut.runtime.server.EmbeddedServer +import io.netty.bootstrap.Bootstrap +import io.netty.channel.ChannelHandlerContext +import io.netty.channel.ChannelInitializer +import io.netty.channel.ChannelOption +import io.netty.channel.SimpleChannelInboundHandler +import io.netty.channel.nio.NioEventLoopGroup +import io.netty.channel.socket.SocketChannel +import io.netty.channel.socket.nio.NioSocketChannel +import io.netty.handler.codec.http.DefaultFullHttpRequest +import io.netty.handler.codec.http.HttpMethod +import io.netty.handler.codec.http.HttpResponse +import io.netty.handler.codec.http.HttpResponseStatus +import io.netty.handler.codec.http.HttpVersion +import io.netty.handler.codec.http2.DefaultHttp2Connection +import io.netty.handler.codec.http2.Http2SecurityUtil +import io.netty.handler.codec.http2.Http2Settings +import io.netty.handler.codec.http2.HttpConversionUtil +import io.netty.handler.codec.http2.HttpToHttp2ConnectionHandlerBuilder +import io.netty.handler.codec.http2.InboundHttp2ToHttpAdapterBuilder +import io.netty.handler.ssl.ApplicationProtocolConfig +import io.netty.handler.ssl.ApplicationProtocolNames +import io.netty.handler.ssl.ApplicationProtocolNegotiationHandler +import io.netty.handler.ssl.SslContextBuilder +import io.netty.handler.ssl.SupportedCipherSuiteFilter +import io.netty.handler.ssl.util.InsecureTrustManagerFactory +import org.jetbrains.annotations.NotNull +import spock.lang.Specification + +import java.time.Instant +import java.time.ZoneOffset +import java.time.format.DateTimeFormatter +import java.util.concurrent.CompletableFuture + +class Http2StaticResourceCacheSpec extends Specification { + def 'response should have proper stream id'() { + given: + def tempFile = File.createTempFile("Http2StaticResourceCacheSpec", ".html") + tempFile.write("HTML Page from static file") + def tempSubDir = new File(tempFile.getParentFile(), "doesntexist") + def app = ApplicationContext.run([ + 'micronaut.server.http-version': '2.0', + 'micronaut.ssl.enabled': true, + 'micronaut.ssl.port': -1, + 'micronaut.ssl.buildSelfSigned': true, + 'micronaut.router.static-resources.default.paths': ['classpath:public', 'file:' + tempFile.parent, 'file:' + tempSubDir.absolutePath] + ]) + def embeddedServer = app.getBean(EmbeddedServer) + embeddedServer.start() + + def sslContext = SslContextBuilder.forClient() + .ciphers(Http2SecurityUtil.CIPHERS, SupportedCipherSuiteFilter.INSTANCE) + .trustManager(InsecureTrustManagerFactory.INSTANCE) + .applicationProtocolConfig(new ApplicationProtocolConfig( + ApplicationProtocolConfig.Protocol.ALPN, + // NO_ADVERTISE is currently the only mode supported by both OpenSsl and JDK providers. + ApplicationProtocolConfig.SelectorFailureBehavior.NO_ADVERTISE, + // ACCEPT is currently the only mode supported by both OpenSsl and JDK providers. + ApplicationProtocolConfig.SelectedListenerFailureBehavior.ACCEPT, + ApplicationProtocolNames.HTTP_2)) + .build() + def completion = new CompletableFuture() + def bootstrap = new Bootstrap() + .remoteAddress(embeddedServer.host, embeddedServer.port) + .group(new NioEventLoopGroup()) + .channel(NioSocketChannel.class) + .option(ChannelOption.AUTO_READ, true) + .handler(new ChannelInitializer() { + @Override + protected void initChannel(@NotNull SocketChannel ch) throws Exception { + def connection = new DefaultHttp2Connection(false) + def connectionHandler = new HttpToHttp2ConnectionHandlerBuilder() + .initialSettings(Http2Settings.defaultSettings()) + .frameListener(new InboundHttp2ToHttpAdapterBuilder(connection) + .maxContentLength(Integer.MAX_VALUE) + .propagateSettings(false) + .build()) + .connection(connection) + .build() + ch.pipeline() + .addLast(sslContext.newHandler(ch.alloc(), embeddedServer.host, embeddedServer.port)) + .addLast(new ApplicationProtocolNegotiationHandler('') { + @Override + protected void configurePipeline(ChannelHandlerContext ctx, String protocol) throws Exception { + if (ApplicationProtocolNames.HTTP_2 != protocol) { + throw new AssertionError((Object) protocol) + } + ctx.pipeline() + .addLast(connectionHandler) + .addLast(new SimpleChannelInboundHandler() { + @Override + protected void channelRead0(ChannelHandlerContext ctx_, HttpResponse msg) throws Exception { + completion.complete(msg) + } + }) + + + def request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, '/' + tempFile.getName()) + request.headers().add(HttpConversionUtil.ExtensionHeaderNames.SCHEME.text(), 'https') + request.headers().add(AbstractNettyHttpRequest.STREAM_ID, 3) + request.headers().add(HttpHeaders.IF_MODIFIED_SINCE, Instant.ofEpochMilli(tempFile.lastModified()).atZone(ZoneOffset.UTC).format(DateTimeFormatter.RFC_1123_DATE_TIME)) + ctx.channel().writeAndFlush(request) + } + }) + } + }) + + when: + def channel = bootstrap.connect().await().channel() + def response = completion.get() + then: + response.status() == HttpResponseStatus.NOT_MODIFIED + response.headers().get(AbstractNettyHttpRequest.STREAM_ID) == '3' + + cleanup: + tempFile.delete() + channel.close() + embeddedServer.close() + } +}