Add h2c prior-knowledge client support#12555
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Netty HTTP client support for h2c prior-knowledge (cleartext HTTP/2 without HTTP/1.1 upgrade), including eager HTTP/2 pipeline initialization and test coverage (integration + negative unit test).
Changes:
- Introduce new
PlaintextMode.H2C_PRIOR_KNOWLEDGEselection inHttpVersionSelection. - Add a Netty
ConnectionManagerinitializer that configures HTTP/2 prior-knowledge connections without waiting for an upgrade/settings handshake. - Add test coverage: an end-to-end embedded-server spec plus a focused negative test against an HTTP/1 peer.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| http-server-netty/src/test/groovy/io/micronaut/http/server/netty/http2/H2cPriorKnowledgeClientSpec.groovy | New embedded-server integration spec validating h2c prior-knowledge for blocking + streaming client APIs. |
| http-client/src/test/groovy/io/micronaut/http/client/netty/ConnectionManagerSpec.groovy | Adds a negative unit test verifying failure behavior against an HTTP/1 peer when configured for h2c prior knowledge. |
| http-client/src/main/java/io/micronaut/http/client/netty/ConnectionManager.java | Implements a dedicated prior-knowledge HTTP/2 channel initializer and adjusts HTTP/2 init flow to support eager init. |
| http-client-core/src/main/java/io/micronaut/http/client/HttpVersionSelection.java | Adds H2C_PRIOR_KNOWLEDGE to the plaintext mode enum. |
| return Flux.create { sink -> | ||
| new Thread({ | ||
| sink.next('f'.getBytes(StandardCharsets.UTF_8)) | ||
| TimeUnit.SECONDS.sleep(1) | ||
| sink.next('oo'.getBytes(StandardCharsets.UTF_8)) | ||
| sink.complete() | ||
| }).start() | ||
| } |
There was a problem hiding this comment.
Spawning a raw new Thread per request and calling sleep without handling interruption/exception can leak threads and leave the Flux uncompleted if the sleep is interrupted (e.g., during shutdown), which can make the test flaky/hang. Prefer using Reactor operators (e.g., delayed emission via scheduler) or, at minimum, wrap the body in try/catch to call sink.error(...) (and restore interrupt flag) on failure.
| private String stream(String url) { | ||
| def composed = new StringBuilder() | ||
| def future = new CompletableFuture<Void>() | ||
| streamingHttpClient.dataStream(HttpRequest.GET(url)).subscribe(new Subscriber<ByteBuffer<?>>() { | ||
| @Override | ||
| void onSubscribe(Subscription s) { | ||
| s.request(Long.MAX_VALUE) | ||
| } | ||
|
|
||
| @Override | ||
| void onNext(ByteBuffer<?> byteBuffer) { | ||
| composed.append(new String(byteBuffer.toByteArray(), StandardCharsets.UTF_8)) | ||
| } | ||
|
|
||
| @Override | ||
| void onError(Throwable t) { | ||
| future.completeExceptionally(t) | ||
| } | ||
|
|
||
| @Override | ||
| void onComplete() { | ||
| future.complete(null) | ||
| } | ||
| }) | ||
| future.get(10, TimeUnit.SECONDS) | ||
| return composed.toString() | ||
| } |
There was a problem hiding this comment.
This manual Subscriber approach makes timeouts/error handling harder: if future.get(...) times out (or the test fails early), the subscription is not cancelled, which can keep network activity running into teardown and increase flakiness. Consider expressing this with reactive operators (collect/join) and a bounded block/timeout so cancellation happens deterministically.
| when: | ||
| conn.clientChannel.close() | ||
| conn.advance() | ||
| future.get() |
There was a problem hiding this comment.
future.get() has no timeout and relies on the method-level @Timeout(30) to stop hangs. Using a bounded wait (e.g., get(… , TimeUnit.SECONDS)) here will fail faster and produce clearer diagnostics when the negative-path behavior regresses.
| future.get() | |
| future.get(5, TimeUnit.SECONDS) |
| return; | ||
| } else { | ||
| log.warn("Premature frame: {}", msg.getClass()); | ||
| ctx.pipeline().remove(ctx.name()); |
There was a problem hiding this comment.
The handler removes itself via ctx.pipeline().remove(ctx.name()), which is more brittle than removing by instance. Prefer ctx.pipeline().remove(this) (or ctx.pipeline().remove(<handler instance>)) to avoid coupling to the handler name and to make the intent clearer.
| ctx.pipeline().remove(ctx.name()); | |
| ctx.pipeline().remove(this); |
|
Blocked on #12624 |
| return Flux.create { sink -> | ||
| new Thread({ | ||
| sink.next('f'.getBytes(StandardCharsets.UTF_8)) | ||
| TimeUnit.SECONDS.sleep(1) |
There was a problem hiding this comment.
This introduces an unconditional 1s real-time sleep per /testStream call, which will slow down the overall test suite (this spec calls /testStream multiple times). Consider reducing the delay substantially (e.g., tens of ms) or using a deterministic mechanism that doesn't rely on wall-clock sleeping while still validating chunking/streaming semantics.
| TimeUnit.SECONDS.sleep(1) | |
| TimeUnit.MILLISECONDS.sleep(10) |
|
|
||
| then: | ||
| def e = thrown ExecutionException | ||
| e.cause != null |
There was a problem hiding this comment.
The assertion only checks that an exception has a cause, which makes the test relatively weak (it will pass for many unrelated failures). Tighten this by asserting the expected exception type and/or a stable part of the message (or a known Netty/Micronaut exception category) so the test better documents and validates the intended failure mode.
| e.cause != null | |
| e.cause instanceof io.micronaut.http.client.exceptions.HttpClientException | |
| e.cause.message != null | |
| e.cause.message.toLowerCase().contains('closed') |
|
Summary
H2C_PRIOR_KNOWLEDGEplaintext client modeVerification
JAVA_HOME=/usr/lib/jvm/java-25-openjdk ./gradlew :micronaut-http-server-netty:test --tests io.micronaut.http.server.netty.http2.H2cPriorKnowledgeClientSpecResolves #10762