Support HTTP/2 connections in REST Client Reactive #31192
Merged
Conversation
This file contains 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
|
@pjgg fyi |
Sgitario
commented
Feb 15, 2023
| @@ -294,6 +294,7 @@ | |||
| <module>rest-client</module> | |||
| <module>resteasy-reactive-kotlin</module> | |||
| <module>rest-client-reactive</module> | |||
| <module>rest-client-reactive-http2</module> | |||
There was a problem hiding this comment.
I know we should avoid adding a new integration test modules, but I think in this case it makes sense since I didn't find any other module where this feature (testing http2 both in server and client side) fits.
Note that the module rest-client-reactive uses Vert.x routes, not the quarkus-resteasy-reactive extension.
Anyway, if you think there is other module that fits better, let me know.
Sgitario
commented
Feb 15, 2023
|
|
||
| public String getHttpVersion() { | ||
| return restClientRequestContext.getVertxClientResponse().version().toString(); | ||
| } |
There was a problem hiding this comment.
To be honest, this was convenient for testing purposes... but I do think it still makes sense as now the HTTP version can be other than HTTP 1.1.
There was a problem hiding this comment.
The problem is that we didn't expose any method to return the HTTP version and I needed it to verify that the HTTP version was the expected in a test.
But in spite of it being convenient for a test, I think it makes sense to keep this method for users as well.
There was a problem hiding this comment.
I seriously doubt anyone will ever use it as it's an implementation class method.
In any case, I'm fine with keeping it
geoand
reviewed
Feb 15, 2023
|
|
||
| [source, properties] | ||
| ---- | ||
| quarkus.rest-client.http2=true |
There was a problem hiding this comment.
Hm, maybe it should be enable-http2? @gsmet WDYT?
There was a problem hiding this comment.
For my understanding, it doesn't make sense to enable HTTP/2 always and let the client negotiate with the server?
There was a problem hiding this comment.
Actually, in vert.x http core this is enabled by default, so it's fine with me to enable http2 by default. Please, confirm if you want me to do this change here.
About, being just http2 or enable-http2, I like more the latter, but in HTTP configuration, it's simply http2. Anyway, let me know what you prefer and I will change it. The same for the alpn property.
There was a problem hiding this comment.
You mean in our http configuration for the server? If so, then let's be consistent
There was a problem hiding this comment.
Yes, so will keep the current 'http2' and 'alpn' names
There was a problem hiding this comment.
I think we probably need @cescoffier 's blessing to enable it by default.
There was a problem hiding this comment.
From my point of view and taking into account the number of failures that enabling http2 by default in REST Client has caused, this flag should be disabled by default.
Though I'm trying to find a solution to the Multipart failures, the solution seems to not be trivial as it involves Vert.x client + Netty client (at least, not trivial to me! Since I don't have much experience with either Vert.x or Netty).
There was a problem hiding this comment.
One thing I don't understand is why this now fails if http2 is enabled by default in Vert.x HTTP client
There was a problem hiding this comment.
I think this is only a client thing that is caused by the QuarkusMultipartFormUpload implementation which internally uses Netty HTTP 1/1. But I'm still investigating.
geoand
reviewed
Feb 15, 2023
| * When the property `http2` is enabled, this flag will be automatically enabled. | ||
| */ | ||
| @ConfigItem | ||
| public Optional<Boolean> alpn; |
There was a problem hiding this comment.
Same comment as the http2 field.
This comment has been minimized.
This comment has been minimized.
|
🙈 The PR is closed and the preview is expired. |
geoand
approved these changes
Feb 16, 2023
|
Having enabled the I'm investigating this. |
I've just found the issue in I need to find a solution to this. |
This comment has been minimized.
This comment has been minimized.
|
The problem seems to be related to subsequent POST calls containing form parts. The first call works, but the second fails. See below a complete traces with further information. |
|
It seems Vert.x fails to find the I also see that there are similar issues to this:
|
|
I've found the root cause of this issue: the http2 decoder fails to validate the HTTP headers in the second call because somehow it has |
|
My guess is that Vert.x is adding the HTTP/2 handlers after the first request is made and then it understand the next requests are HTTP/2 requests, so it fails because the By the way, to reproduce this issue, you can build the changes in this pull request (
However, when we don't set the After this change, the only test that keeps failing is: |
This comment has been minimized.
This comment has been minimized.
|
Reported issue in Vert.x: eclipse-vertx/vert.x#4618 |
|
I've just updated the pull request and fixed all the issues with Vert.x which I reported:
I could workaround this issue by manually adapting the request (the only issues were about the
This is a minor issue, but users would notice a different behavior. At the moment, we cannot map the HttpClosedException to remap it to a TimeoutException as it's happening in HTTP 1.1. |
This comment has been minimized.
This comment has been minimized.
|
The failures are caused by eclipse-vertx/vert.x#4630 and eclipse-vertx/vert.x#4631. |
This comment has been minimized.
This comment has been minimized.
|
The |
|
@geoand @gsmet @cescoffier I've fixed all the issues we got with using HTTP2 in RESTeasy reactive client, so it should be good to proceed.
Therefore, are you good to disable HTTP2 by default and proceed with this pull request? |
|
👌 |
|
I will wait for you all before disabling http2 by default in the reactive rest-client |
This comment has been minimized.
This comment has been minimized.
|
@Sgitario I agree it's the best solution for now. /cc @cescoffier for further validation of what @Sgitario is saying here: #31192 (comment) . |
|
Yea, go ahead and disable by default. |
Sgitario
commented
Feb 27, 2023
| @@ -343,6 +343,11 @@ public void setContentTransferEncoding(String contentTransferEncoding) { | |||
| this.contentTransferEncoding = contentTransferEncoding; | |||
| } | |||
|
|
|||
| @Override | |||
| public long length() { | |||
| return buffer.readableBytes() + super.length(); | |||
There was a problem hiding this comment.
The content-length was missing the buffer bytes. HTTP/1.1 was ok with this, but HTTP/2 is much stricter.
Sgitario
commented
Feb 27, 2023
| counter.incrementAndGet(); | ||
| boolean writable = pending.write(buff); | ||
| if (encoder.isEndOfInput()) { | ||
| } else if (chunk == LastHttpContent.EMPTY_LAST_CONTENT || encoder.isEndOfInput()) { |
There was a problem hiding this comment.
This will save up to send an empty request to the server.
|
PR updated by:
|
This comment has been minimized.
This comment has been minimized.
Failing Jobs - Building a9a6c97
Full information is available in the Build summary check run. Failures⚙️ Maven Tests - JDK 11 #- Failing: integration-tests/maven
📦 integration-tests/maven✖ ✖ ⚙️ Maven Tests - JDK 11 Windows #📦 integration-tests/maven✖ ✖ |
gsmet
approved these changes
Mar 2, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Relates #13969