Http2FrameListener-based HTTP/2 server handler implementation #10596
Conversation
In a few places, I used the pattern where reactive calls would be dispatched to the event loop if they weren't already on the loop. However, if there is a reactive call outside the event loop, and then immediately a reactive call *on* the event loop, the calls can run in the wrong order. The second call on the event loop was not delayed with an execute() call, so it could run before the first call runs. This fix introduces a central EventLoopSerializer that takes care of this problem. In the above problem scenario, it takes care to also delay the second call even though it was submitted on the event loop.
# Conflicts: # http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/PipeliningServerHandler.java
# Conflicts: # http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/PipeliningServerHandler.java
| * As of 4.4.0, this approach was replaced with a more performant HTTP/2-specific | ||
| * implementation. This means worse compatibility with HTTP/1.1-based code paths and | ||
| * customizers, however. Setting this option to {@code true} returns to the old behavior. |
There was a problem hiding this comment.
Does this mean that unless this is true, Http/1.1 services will notice a slowdown?
Are we going to keep this as true for the forseeable future? (I see the field is marked "TODO") 🤔
It's also something to rememeber if there's Http/2 benchmarks out there...
There was a problem hiding this comment.
no, http/1.1 code paths are basically unchanged
i plan to make this false by default when compression and maybe server push is implemented
| s.onSubscribe(new Subscription() { | ||
| @Override | ||
| public void request(long n) { | ||
| synchronized (DelayedSubscriber.this) { |
There was a problem hiding this comment.
can we use read/write locks for all these synchronised blocks?
There was a problem hiding this comment.
yes but this sync block is fine for loom because it only guards memory access (no big function calls or IO)
| } else if (demand != 0) { | ||
| upstream.request(demand); | ||
| } else { | ||
| assert cancel; |
There was a problem hiding this comment.
this requires enabling assertions in the JDK, should this be an illegal statue exception?
There was a problem hiding this comment.
cancel is always true here because if none of the branches are hit, we would have returned above. intellij complained when i had if (cancel) here because it was able to figure out that cancel was always true, so i changed it to an assert.
| @@ -116,6 +116,7 @@ enum ChannelRole { | |||
| /** | |||
| * The channel is a channel representing an individual HTTP2 stream. | |||
| */ | |||
| // todo: deprecate | |||
There was a problem hiding this comment.
can only deprecate when the new impl is actually enabled by default
There was a problem hiding this comment.
You can put @NextMajorVersion
There was a problem hiding this comment.
i want to deprecate it in 4.5 already
http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/Http2ServerHandler.java
Outdated
Show resolved
Hide resolved
http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/Http2ServerHandler.java
Outdated
Show resolved
Hide resolved
http-server-netty/src/main/java/io/micronaut/http/server/netty/handler/Http2ServerHandler.java
Outdated
Show resolved
Hide resolved
# Conflicts: # http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyEmbeddedServices.java
core-reactive/src/main/java/io/micronaut/core/async/publisher/DelayedSubscriber.java
Show resolved
Hide resolved
core-reactive/src/main/java/io/micronaut/core/async/publisher/DelayedSubscriber.java
Outdated
Show resolved
Hide resolved
|
| @@ -196,6 +196,7 @@ public class NettyHttpServerConfiguration extends HttpServerConfiguration { | |||
| private List<NettyListenerConfiguration> listeners = null; | |||
| private boolean eagerParsing = DEFAULT_EAGER_PARSING; | |||
| private int jsonBufferMaxComponents = DEFAULT_JSON_BUFFER_MAX_COMPONENTS; | |||
| private boolean legacyMultiplexHandlers = true; // TODO | |||
There was a problem hiding this comment.
i want to change this in 4.5
Another feature for #10596. Once again no separate tests, this functionality is covered by Http2ServerPushSpec when legacy-multiplex-handlers is off.
Another feature for #10596. The session ID test fails because micronaut-session uses a channel attribute that we cannot support with HTTP/2. It's flawed even for HTTP/1, but would probably need an API change to fix.
Another feature for #10596. Once again no separate tests, this functionality is covered by Http2ServerPushSpec when legacy-multiplex-handlers is off.
Another feature for #10596. The session ID test fails because micronaut-session uses a channel attribute that we cannot support with HTTP/2. It's flawed even for HTTP/1, but would probably need an API change to fix.
Before this patch, HTTP/2 was implemented by taking a netty Http2MultiplexHandler, which creates a new netty channel for each HTTP/2 stream, and adding the HTTP/1.1 PipeliningServerHandler to each of those stream channels. This introduced unnecessary overhead due to the channel creation and the unnecessary features from PipeliningServerHandler.
This patch instead implements a Http2FrameListener directly, which is the lowest level netty HTTP/2 abstraction. This avoids the channel creation and simplifies some parts compared to PipeliningServerHandler (e.g. backpressure and no more need for pipelining). The implementation is split in two classes (MultiplexedServerHandler and Http2ServerHandler) in order to accommodate HTTP/3 in the future.
The new server handler has some missing features that I plan to add in future PRs, to keep this PR as minimal as possible:
When these changes are implemented, I plan to enable the new approach by default, likely in 4.4.0 still.
This new approach also removes some possibility for user pipeline customization. The only thing in the netty pipeline now, after the SSL handler, is the HTTP/2 decoder. It directly passes the HTTP/2 frames to the Micronaut Http2ServerHandler, without an intermediate step to HTTP/1.1 (Http2ServerHandler still converts the messages to HTTP/1.1 internally, so that RoutingInboundHandler can remain as-is, but the messages are not sent on any netty pipeline as before). This means e.g. logbook will not work. For this reason I still plan to keep the option of reverting to the old multiplexing approach, at least until 5.0.0. It is fairly low-maintenance anyway.