Replies: 2 comments
-
Hi James, thanks for the discussion. I have a few misc comment for now:
|
Beta Was this translation helpful? Give feedback.
0 replies
-
Hello Alessio,
On Fri, Aug 11, 2023 at 3:25 AM Alessio Soldano ***@***.***> wrote:
Hi James, thanks for the discussion. I have a few misc comment for now:
- do we know the exact implications of HttpClient not being closeable
till Java 21 for the most common usecases and can we explain them to users?
Logging a warning message for this as well (perhaps once for each
deployment) might make sense
The theory is it uses the java.lang.ref.Cleaner and closes the connection
when the client is no longer reachable. That does present the problem of
having to create a new HttpClient for every request. However, that seems
the intent of the API. I think they listened to the community though for 21
and realized it was a good idea to allow closing. It's not always easy to
ensure the client reference is not left somewhere.
From my naive testing I've not seen connections left open too long or not
being closed at all. The closing in some cases even seems a bit aggressive
as a guess. I think this is why CI is failing on the job.
- I'm fine with the proposed approach for the hostname verifier,
hopefully the spec can be modified btw
I should have linked it, but I did propose that
jakartaee/rest#1161. There is always a chance the
JDK HttpClient will add it, but I'm not too confident in that. I can
understand why they don't want it, but for testing purposes it does make
sense. It can be disabled globally by setting
the jdk.internal.httpclient.disableHostnameVerification system property to
false. However, for production that doesn't make sense.
…
- I assume this big change would go into a major release, right?
Yes. I'm hoping for RESTEasy 4.0, which would target Jakarta REST 4.0.
- offering a property for controlling the default ClientHttpEngine
impl to use looks fine, given the implications above
Makes sense to me as well, thank you. I'll add that for sure.
—
Reply to this email directly, view it on GitHub
<#3743 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADGRYOM4GJ2OI2JY6MU2PTXUYB7ZANCNFSM6AAAAAA3KKL4QA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
James R. Perkins
JBoss by Red Hat
|
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
-
Note this is a cross post: https://lists.jboss.org/archives/list/resteasy-dev@lists.jboss.org/thread/QXDIISJWTFTYJGUYSBB4AKT3V4IE32J6/
Hello All,
Though it's currently failing CI and is not complete, but I would like to get opinions on changing the default backing HTTP Client for the RESTEasy REST Client to use the JDK's HttpClient. See the PR for the implementation changes. This is something I've been working off and on again since last December. It feels like it's time to make some decisions
before I spend any more time on it :) FWIW it passes for me locally with all profiles on Java 11 on Linux.
One of the big things this gives us would be HTTP/2 support. There are some caveats
however.
One is that the
HostnameVerifier
cannot be set on thejava.net.http.HttpClient
. My simple solution to that was to use the old Apache HTTP client if that is the case and log message indicating that.Another "issue" is that the
HttpClient
isn't closeable. However, in Java 21 this changes so it might not be a big issue. Jakarta REST 4.0 might require Java 21.Some tests also required changes. Some only worked with HTTP 1.1 for some reason and others just seemed to require the Apache Client.
I do think a property should be added for a general fallback to the Apache Client. There could be applications that assume the
ClientHttpEngine
is assumed to be aApacheHttpClientEngine
. We shouldn't require the application to define the engine in the builder each time it creates a client.In general I'd like to hear what others think about this. I do think having a client that supports HTTP/2 is a must. Either way, we likely break some assumptions about what the default
ClientHttpEngine
is. Please let me know; good, bad, indifferent or if we should use a different client all together.Beta Was this translation helpful? Give feedback.
All reactions