Repository navigation
Add HTTP/2 support for the Reactor Netty integration - #2325
sri-adarsh-kumar wants to merge 6 commits into
Conversation
kasmarian
left a comment
kasmarian
left a comment
There was a problem hiding this comment.
Thank you for your contribution! I like that Logbook will have a good HTTP2 support with it.
| </dependency> | ||
| <dependency> | ||
| <groupId>io.netty</groupId> | ||
| <artifactId>netty-codec-http2</artifactId> |
There was a problem hiding this comment.
Wouldn't adding it as not an optional dependency mean that every consumer of logbook-netty, including the overwhelming majority who only use HTTP/1.1, will get netty-codec-http2 added to their transitive dependency tree unconditionally. This is unnecessary classpath bloat and breaks the principle
that optional features should not be forced on all users.
There was a problem hiding this comment.
Agreed. netty-codec-http2 is now optional, and HTTP/2-only type access is isolated behind a lazy boundary. An isolated HTTP/1.1 regression test also verifies logging works when the HTTP/2 codec and Reactor Netty are physically absent.
| import static org.mockito.Mockito.verify; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| class Http2AwareHandlerRegistrarTest { |
There was a problem hiding this comment.
Correct me if I'm wrong, but this test doesn't seem to test that HTTP2 works as expected. Which is the main point to ensure to have no regression afterwards. Please add tests that'd use and show "HTTP/2.0" protocol in log assertions. Could be done in a separate set of test files, if needed.
There was a problem hiding this comment.
Agreed. The suite now runs real H2C client/server exchanges and asserts HTTP/2.0 in request and response logs on both sides, including bodies, preserved user headers, and removed synthetic headers.
| */ | ||
| public static HttpServer installOnServer(final HttpServer httpServer, final Logbook logbook) { | ||
| return httpServer.observe((connection, state) -> { | ||
| if (state == ConnectionObserver.State.CONFIGURED) { |
There was a problem hiding this comment.
In case of the client, you also check if state == HttpClientState.STREAM_CONFIGURED, why is it not needed here to check if state == HttpServerState.CONFIGURED? I'm unfamiliar with this level of netty internals, please excuse me if it's something obvious.
There was a problem hiding this comment.
Reactor Netty has no HttpServerState.CONFIGURED; its server-specific states are request-level states. Generic ConnectionObserver.State.CONFIGURED is emitted for both HTTP/1.1 channels and HTTP/2 stream channels. The implementation uses childObserve, retains the duplicate-handler guard, and covers this with focused and real-traffic tests.
|
|
||
| @Test | ||
| void shouldBeDefaultRequest() { | ||
|
|
||
| HttpRequest req = mock(HttpRequest.class); | ||
| when(req.uri()).thenReturn("/test?a=b"); | ||
| when(req.headers()).thenReturn(new DefaultHttpHeaders().add(CONTENT_TYPE, "text/plain")); | ||
| when(req.method()).thenReturn(HttpMethod.GET); | ||
| when(req.protocolVersion()).thenReturn(HttpVersion.HTTP_1_1); | ||
|
|
||
| ChannelHandlerContext context = mockChannelHandlerContext(); | ||
| HttpRequest req = request("/test?a=b", headers(CONTENT_TYPE.toString(), "text/plain")); | ||
| EmbeddedChannel channel = channel(null, LocalAddress.ANY, sslHandler()); |
There was a problem hiding this comment.
I have concerns with the change in the logic of getScheme() implementation. Now it's first check if the parent channel exist and take the SslHandler of it. Can it be a breaking change for HTTP1.1 clients that have parent channel without the SslHandler?
A test to simulate this would be
@Test
void shouldReturnHttpsSchemeForHttp11ServerTlsWhereOnlyChildChannelHasSslHandler() {
EmbeddedChannel parent = channel(null, null);
EmbeddedChannel child = channel(parent, LocalAddress.ANY, sslHandler());
Request request = new Request(context(child), REMOTE, request("/", new DefaultHttpHeaders()));
assertThat(request.getScheme()).isEqualTo("https");
}
There was a problem hiding this comment.
And while we're at it, maybe worth having a dedicated test for HTTP2 flow as well
@Test
void shouldReturnHttpsSchemeForHttp2StreamChannelWhoseParentHasSslHandler() {
EmbeddedChannel parent = channel(null, null, sslHandler());
EmbeddedChannel h2stream = http2Channel(parent, LocalAddress.ANY);
Request request = new Request(context(h2stream), REMOTE, request("/", new DefaultHttpHeaders()));
assertThat(request.getScheme()).isEqualTo("https");
}
There was a problem hiding this comment.
Good catch. getScheme() now checks the current channel first, preserving HTTP/1.1 child-channel TLS. I've also added a dedicated HTTP/2 stream test with TLS on the parent channel, matching Reactor Netty's topology.
Add isolated optional-dependency regressions, real H2C acceptance coverage, complete synthetic-header cleanup, TLS scheme preservation, and integration guidance. Co-Authored-By: gpt-5.6-sol <noreply@anthropic.com>
…mar/logbook into add-http2-support
Co-Authored-By: github-copilot/gpt-5.6-sol <noreply@anthropic.com>
|
@kasmarian small reminder to resume review here |
|
Sorry for not answering, I've also asked my colleagues to have a look at it to |
|
@kasmarian Reminder for this review |
2 participants
Fixes #961
Summary
Http2AwareHandlerRegistrarso Logbook handlers are installed on the right Reactor Netty lifecycle hooks for both HTTP/1.1 and HTTP/2 streamsHTTP/2.0, resolving the scheme from the parent TLS channel, and stripping synthetic HTTP/2 conversion headers from logged headersMotivation
The previous
doOnConnected/doOnConnectionregistration pattern works for HTTP/1.1 but misses HTTP/2 stream channels, so Logbook does not reliably instrument multiplexed Reactor Netty traffic. This change makes the recommended integration path HTTP/2-aware without regressing HTTP/1.1.Test Plan
./mvnw test -pl logbook-netty -am