From 8a0c55482b1adadf5cc74d73b297323a6447841a Mon Sep 17 00:00:00 2001 From: yawkat Date: Wed, 17 Apr 2024 13:59:30 +0200 Subject: [PATCH 1/2] Enable Http2ServerHandler by default Also two minor bug fixes related to h2c. --- .../server/netty/NettyServerCustomizer.java | 9 +++++++- .../NettyHttpServerConfiguration.java | 2 +- .../netty/handler/Http2ServerHandler.java | 14 +++++++++--- .../handler/MultiplexedServerHandler.java | 4 +++- .../Http2AccessLogFrameListener.java | 19 +--------------- .../accesslog/Http2AccessLogManager.java | 22 +++++++++++++++++++ .../nettyServerPipeline.adoc | 2 +- 7 files changed, 47 insertions(+), 25 deletions(-) diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyServerCustomizer.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyServerCustomizer.java index a377e5a0a2f..d8646c0ccdd 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyServerCustomizer.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyServerCustomizer.java @@ -115,11 +115,18 @@ enum ChannelRole { CONNECTION, /** * The channel is a channel representing an individual HTTP2 stream. + *

+ * Note: As of 4.5.0, there is no separate channel for each request anymore for performance + * reasons. You can revert to the old behavior using the + * {@code micronaut.server.netty.legacy-multiplex-handlers=true} configuration property. */ - // todo: deprecate REQUEST_STREAM, /** * The channel is a channel representing an individual HTTP2 stream, created for a push promise. + *

+ * Note: As of 4.5.0, there is no separate channel for each request anymore for performance + * reasons. You can revert to the old behavior using the + * {@code micronaut.server.netty.legacy-multiplex-handlers=true} configuration property. */ PUSH_PROMISE_STREAM, } diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/configuration/NettyHttpServerConfiguration.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/configuration/NettyHttpServerConfiguration.java index e41a2992e71..f372ac6306c 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/configuration/NettyHttpServerConfiguration.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/configuration/NettyHttpServerConfiguration.java @@ -196,7 +196,7 @@ public class NettyHttpServerConfiguration extends HttpServerConfiguration { private List listeners = null; private boolean eagerParsing = DEFAULT_EAGER_PARSING; private int jsonBufferMaxComponents = DEFAULT_JSON_BUFFER_MAX_COMPONENTS; - private boolean legacyMultiplexHandlers = true; // TODO + private boolean legacyMultiplexHandlers = false; /** * Default empty constructor. diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/Http2ServerHandler.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/Http2ServerHandler.java index 99e9e25c166..c1ecf1dce28 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/Http2ServerHandler.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/Http2ServerHandler.java @@ -61,6 +61,7 @@ public final class Http2ServerHandler extends MultiplexedServerHandler implement private Http2ConnectionHandler connectionHandler; private Http2Connection.PropertyKey streamKey; private boolean reading = false; + private boolean upgradedFromHttp1 = false; static { for (Http2Error value : Http2Error.values()) { @@ -192,10 +193,13 @@ public void onUnknownFrame(ChannelHandlerContext ctx, byte frameType, int stream */ public static final class ConnectionHandler extends Http2ConnectionHandler { private final Http2ServerHandler handler; + @Nullable + private final Http2AccessLogManager accessLogManager; - private ConnectionHandler(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder, Http2Settings initialSettings, boolean decoupleCloseAndGoAway, boolean flushPreface, Http2ServerHandler handler) { + private ConnectionHandler(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder, Http2Settings initialSettings, boolean decoupleCloseAndGoAway, boolean flushPreface, Http2ServerHandler handler, Http2AccessLogManager accessLogManager) { super(decoder, encoder, initialSettings, decoupleCloseAndGoAway, flushPreface); this.handler = handler; + this.accessLogManager = accessLogManager; } @Override @@ -238,7 +242,11 @@ protected void handlerRemoved0(ChannelHandlerContext ctx) throws Exception { @Override public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exception { if (evt instanceof HttpServerUpgradeHandler.UpgradeEvent upgrade) { + handler.upgradedFromHttp1 = true; FullHttpRequest fhr = upgrade.upgradeRequest(); + if (accessLogManager != null) { + accessLogManager.logHeaders(ctx, 1, fhr); + } io.netty.handler.codec.http2.Http2Stream cs = connection().stream(1); handleFakeRequest(cs, fhr); } @@ -320,7 +328,7 @@ protected ConnectionHandler build(Http2ConnectionDecoder decoder, Http2Connectio if (accessLogManager != null) { encoder = new Http2AccessLogConnectionEncoder(encoder, accessLogManager); } - ConnectionHandler ch = new ConnectionHandler(decoder, encoder, initialSettings, decoupleCloseAndGoAway(), flushPreface(), frameListener); + ConnectionHandler ch = new ConnectionHandler(decoder, encoder, initialSettings, decoupleCloseAndGoAway(), flushPreface(), frameListener, accessLogManager); frameListener.init(ch); return ch; } @@ -336,7 +344,7 @@ private final class Http2Stream extends MultiplexedStream { @Override void notifyDataConsumed(int n) { - if (stream.id() == 1) { + if (stream.id() == 1 && upgradedFromHttp1) { // ignore for upgrade stream return; } diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/MultiplexedServerHandler.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/MultiplexedServerHandler.java index f4fc659c934..7c925c8dc08 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/MultiplexedServerHandler.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/MultiplexedServerHandler.java @@ -197,7 +197,9 @@ final void devolveToStreaming() { streamer = new InputStreamer(); if (bufferedContent != null) { for (ByteBuf buf : bufferedContent) { - streamer.sink.tryEmitNext(new DefaultHttpContent(buf)); + if (streamer.sink.tryEmitNext(new DefaultHttpContent(buf)) != Sinks.EmitResult.OK) { + buf.release(); + } } bufferedContent = null; } diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/accesslog/Http2AccessLogFrameListener.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/accesslog/Http2AccessLogFrameListener.java index 3a5fe84038a..aa58b10e0da 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/accesslog/Http2AccessLogFrameListener.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/accesslog/Http2AccessLogFrameListener.java @@ -16,9 +16,7 @@ package io.micronaut.http.server.netty.handler.accesslog; import io.micronaut.core.annotation.Internal; -import io.micronaut.http.server.netty.handler.accesslog.element.AccessLog; import io.netty.channel.ChannelHandlerContext; -import io.netty.channel.socket.SocketChannel; import io.netty.handler.codec.http.HttpRequest; import io.netty.handler.codec.http2.Http2Exception; import io.netty.handler.codec.http2.Http2FrameListener; @@ -42,23 +40,8 @@ public Http2AccessLogFrameListener(Http2FrameListener listener, Http2AccessLogMa } private void logHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers headers) throws Http2Exception { - if (!manager.logger.isInfoEnabled()) { - return; - } HttpRequest request = HttpConversionUtil.toHttpRequest(streamId, headers, false); - if (manager.uriInclusion != null && !manager.uriInclusion.test(request.uri())) { - return; - } - - AccessLog accessLog; - if (manager.logForReuse != null) { - accessLog = manager.logForReuse; - manager.logForReuse = null; - } else { - accessLog = manager.formatParser.newAccessLogger(); - } - manager.connection.stream(streamId).setProperty(manager.accessLogKey, accessLog); - accessLog.onRequestHeaders((SocketChannel) ctx.channel(), request.method().name(), request.headers(), request.uri(), HttpAccessLogHandler.H2_PROTOCOL_NAME); + manager.logHeaders(ctx, streamId, request); } @Override diff --git a/http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/accesslog/Http2AccessLogManager.java b/http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/accesslog/Http2AccessLogManager.java index a882014513e..6a9fbeca502 100644 --- a/http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/accesslog/Http2AccessLogManager.java +++ b/http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/accesslog/Http2AccessLogManager.java @@ -18,6 +18,9 @@ import io.micronaut.core.annotation.Internal; import io.micronaut.http.server.netty.handler.accesslog.element.AccessLog; import io.micronaut.http.server.netty.handler.accesslog.element.AccessLogFormatParser; +import io.netty.channel.ChannelHandlerContext; +import io.netty.channel.socket.SocketChannel; +import io.netty.handler.codec.http.HttpRequest; import io.netty.handler.codec.http2.Http2Connection; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -49,6 +52,25 @@ public Http2AccessLogManager(Factory factory, Http2Connection connection) { this.uriInclusion = factory.uriInclusion; } + public void logHeaders(ChannelHandlerContext ctx, int streamId, HttpRequest request) { + if (!logger.isInfoEnabled()) { + return; + } + if (uriInclusion != null && !uriInclusion.test(request.uri())) { + return; + } + + AccessLog accessLog; + if (logForReuse != null) { + accessLog = logForReuse; + logForReuse = null; + } else { + accessLog = formatParser.newAccessLogger(); + } + connection.stream(streamId).setProperty(accessLogKey, accessLog); + accessLog.onRequestHeaders((SocketChannel) ctx.channel(), request.method().name(), request.headers(), request.uri(), HttpAccessLogHandler.H2_PROTOCOL_NAME); + } + public record Factory(Logger logger, String spec, Predicate uriInclusion) { } } diff --git a/src/main/docs/guide/httpServer/serverConfiguration/nettyServerPipeline.adoc b/src/main/docs/guide/httpServer/serverConfiguration/nettyServerPipeline.adoc index 8e4a43ab6ac..dd482e1ce23 100644 --- a/src/main/docs/guide/httpServer/serverConfiguration/nettyServerPipeline.adoc +++ b/src/main/docs/guide/httpServer/serverConfiguration/nettyServerPipeline.adoc @@ -12,4 +12,4 @@ snippet::io.micronaut.docs.netty.LogbookNettyServerCustomizer[tags="imports,clas <4> When a new channel is created, a new, specialized customizer is created for that channel <5> When the server signals that the stream pipeline has been fully constructed, the logbook handler is registered -WARNING: Logbook has a https://github.com/zalando/logbook/issues/1216[major bug] that limits its usefulness with netty. +WARNING: As of version 4.5.0, the Micronaut netty HTTP server does not use per-request channels for HTTP/2 anymore. This makes many pipeline modifications (including logbook) difficult or impossible to implement. To revert to the old behavior, use the `micronaut.server.netty.legacy-multiplex-handlers=true` property. From d634d312f358aa64422cde7ced7514012fb78d1b Mon Sep 17 00:00:00 2001 From: yawkat Date: Wed, 17 Apr 2024 15:31:46 +0200 Subject: [PATCH 2/2] move test to PendingFeatureIf --- .../test/groovy/io/micronaut/http/HttpAccessLogger.groovy | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test-suite/src/test/groovy/io/micronaut/http/HttpAccessLogger.groovy b/test-suite/src/test/groovy/io/micronaut/http/HttpAccessLogger.groovy index 2ba0eae399d..91c10c58aa0 100644 --- a/test-suite/src/test/groovy/io/micronaut/http/HttpAccessLogger.groovy +++ b/test-suite/src/test/groovy/io/micronaut/http/HttpAccessLogger.groovy @@ -29,7 +29,7 @@ import io.micronaut.test.extensions.spock.annotation.MicronautTest import jakarta.inject.Inject import org.slf4j.LoggerFactory import reactor.core.publisher.Flux -import spock.lang.IgnoreIf +import spock.lang.PendingFeatureIf import spock.lang.Specification /** @@ -108,8 +108,9 @@ class HttpAccessLoggerSpec extends Specification { } - @IgnoreIf({ instance instanceof Http2AccessLoggerSpec }) // micronaut-session uses channel attributes to get the request, which is broken - void "test simple session - access logger"() { + @PendingFeatureIf({ instance instanceof Http2AccessLoggerSpec }) + // micronaut-session uses channel attributes to get the request, which is broken + void "test simple session - access logger"() { when: appender.events.clear() Flux> flowable = Flux.from(client.exchange(