Support HTTP/3 in Jetty and JRE HTTP Client#1949
Open
kwin wants to merge 12 commits into
Open
Conversation
kwin
force-pushed
the
feature/http3
branch
2 times, most recently
from
95b0e32 to
0dd1244
Compare
Consolidate http version configuration among HTTP transporters Default to HTTP/2 in all transporters except Apache HTTP Client 4.x Still WIP
Member
Author
|
@vy Maybe you can have a look. Connecting from Java HTTP Client via HTTP3 to a local Jetty HTTP/3 server fails in Logging enabled with The same endpoint works fine with Jetty HTTP/3 client. |
kwin
force-pushed
the
feature/http3
branch
2 times, most recently
from
c2376c1 to
612993d
Compare
Member
|
feature-http3-jdk-fix.patch — contributed by @dfuch and @jaikiran — fixes
|
Member
Author
|
@vy Thanks a lot. I raised jetty/jetty.project#15385 because the Quiche binding missing currently is just silently discarded in Jetty. I meanwhile figured out how to properly connect with JRE HTTP Client to Jetty via HTTP/3. However I am still missing an insecure option (to completely skip the server's certificate check) |
Clean up and regenerate key stores Cleanup server method names
kwin
force-pushed
the
feature/http3
branch
2 times, most recently
from
cf7c79d to
8aa84dd
Compare
kwin
force-pushed
the
feature/http3
branch
2 times, most recently
from
8daa9e2 to
59de191
Compare
Member
Author
|
@cstamas I don't understand the unstable test |
kwin
commented
Jul 9, 2026
| * | ||
| * @configurationSource {@link RepositorySystemSession#getConfigProperties()} | ||
| * @configurationType {@link ConfigurationProperties.HttpVersion} | ||
| * @configurationDefaultValue {@link #DEFAULT_HTTP_VERSION} |
Member
Author
There was a problem hiding this comment.
@cstamas This constant cannot be extracted with
. Any idea how to fix this?errors with Jetty
Always consume request body to prevent connection resets
gnodet
reviewed
Jul 13, 2026
gnodet
left a comment
gnodet
left a comment
Contributor
There was a problem hiding this comment.
AI Review — PR #1949: Support HTTP/3 in Jetty and JRE HTTP Client
Hi @kwin, great work on bringing HTTP/3 support to the resolver! The consolidated HttpVersion enum and test infrastructure refactoring are well-designed.
🔴 SSLContext test safety (high)
testGet_HTTPS_Insecure_SecurityMode calls SSLContext.setDefault(defaultSslContext) at the start and restores it at the end, but without a try/finally block. If any assertion throws mid-test, the SSLContext remains corrupted for subsequent tests. This likely contributes to the CI failures: testPut_SSL fails with HTTP 500 in JdkTransporterTest on 3/6 JDK 21 matrix jobs. With @TestMethodOrder(MethodOrderer.MethodName.class), the insecure test runs before testPut_SSL alphabetically. The fix: wrap the test body in try/finally to guarantee SSLContext restoration.
⚠️ getEnum unchecked cast & Javadoc errors (medium)
ConfigUtils.getEnum uses (T) value (unchecked cast) — should use enumClass.isInstance(value) + enumClass.cast(value) for type safety. Both getEnum overloads have copy-paste Javadoc errors: @param defaultValue says "boolean" instead of "enum", and the session overload has duplicate @param session tags.
⚠️ HTTP/3 objects always allocated (medium)
QuicheClientQuicConfiguration, HTTP3Client, and QuicheTransport are instantiated unconditionally in JettyTransporter.createClient(), even when httpVersion is DEFAULT or HTTP_2. These involve native Quiche resources. Moving construction inside the HTTP_3/MAXIMUM switch branches would avoid unnecessary allocation in the common case.
⚠️ Missing HTTP/2 fallback for HTTP/3 (medium)
A comment says "always support HTTP/2 as fallback for HTTP/3" but when HTTP_3 or MAXIMUM is selected, only http3 is added to connectors — http2 is not. Either the code or the comment needs updating.
What works well:
- The
HttpVersionenum consolidating HTTP version config across all transport backends is clean API design - The
HttpServertest refactoring simplifies setup meaningfully - Good defensive additions: null-entity check in
ApacheRFC9457Reporter.getBody(), warn-level log before deleting failed files
🤖 This review was generated by ForgeBot using a maker/checker pattern (reviewer + independent verifier). All 4 key findings were independently confirmed.
Member
Author
This happens due to jetty/jetty.project#13157 (comment) and is not really related to the HTTP3 feature introduced in this feature, however previously the test was being performed with HTTP/1.1 only and not with HTTP/2. |
Incorporate feedback from PR review Add test for HTTP3 -> HTTP2 fallback
kwin
force-pushed
the
feature/http3
branch
3 times, most recently
from
70a2d92 to
da0b0a5
Compare
Those ship with an incompatible GLIBC version not supported by the Jetty Quiche Library (jetty-project/jetty-quiche-native#180)
gnodet
reviewed
Jul 15, 2026
gnodet
left a comment
gnodet
left a comment
Contributor
There was a problem hiding this comment.
Review Summary
Nice work on this, @kwin! The HTTP/3 support and HTTP version configuration consolidation is well-structured. The shared HttpVersion enum in the API module with centralized getHttpVersion utility and transporter-specific mapping is a clean architecture. CI is green and the backward compatibility approach for the JDK transporter's legacy property is well thought out.
A few Javadoc issues to address:
Medium Severity
-
Broken Javadoc link in
JdkTransporterConfigurationKeys—{@link ConfigProperties#CONFIG_PROP_HTTP_VERSION}references a non-existent classConfigProperties. The correct class name isConfigurationProperties. -
Missing Javadoc on new public method —
getHttpVersioninHttpTransporterUtilsis a new public static method but has no Javadoc. Other public methods in this class (e.g.,getHttpLocalAddress) have proper Javadoc with@sincetags.
Low Severity
-
Duplicate
@param sessionin the session-basedgetEnumoverload inConfigUtils.java— the parameter appears twice in the Javadoc. -
Copy-paste Javadoc error — Both
getEnumoverloads say@param defaultValue"in case none of the property keys is set to a boolean" — should say "enum" (copy-pasted fromgetBoolean). -
Dead code —
case DEFAULT:inJdkTransporter.getHttpVersion()switch (line 490) is unreachable because theDEFAULTenum value is already handled by theifguard before the switch is reached. Not a bug, just dead code.
General Notes
- The Jetty HTTP/3 limitation (no fallback to HTTP/2) is properly documented.
- The
server-store-selfsignedkeystore consolidation is a welcome cleanup. - The PR description says "Still WIP" with all checklist items unchecked — consider updating the description or marking as draft when the PR is not yet ready for merge.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
gnodet
reviewed
Jul 15, 2026
gnodet
left a comment
gnodet
left a comment
Contributor
There was a problem hiding this comment.
Follow-up Review — All Prior Findings Addressed ✅
Thanks for the quick fix, @kwin! I've re-reviewed after commit 8b3495dc ("Incorporate PR feedback") and can confirm all 5 findings from my prior review are resolved:
- ✅ Broken Javadoc link —
ConfigProperties#CONFIG_PROP_HTTP_VERSION→ConfigurationProperties#HTTP_VERSIONinJdkTransporterConfigurationKeys.java - ✅ Missing Javadoc —
getHttpVersioninHttpTransporterUtils.javanow has proper Javadoc - ✅ Duplicate
@param session— removed inConfigUtils.javagetEnumoverload - ✅ Copy-paste "boolean" → "enum" — both
getEnumoverloads now correctly say "convertible String or T" - ✅ Dead code
DEFAULTcase — merged withHTTP_2fall-through;defaulthas explanatory comment and usesIllegalStateException(more semantically correct)
No new issues found in the fix commit. The PR looks good from a code quality perspective.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidate http version configuration among HTTP transporters Default to HTTP/2 in all transporters except Apache HTTP Client 4.x
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
mvn -Prun-its verify).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.