Implement HttpClient support for an overall request timeout
Description
Environment
Activity
Brent Putman December 6, 2024 at 4:48 AM
Ok I have changed the default pool size to 20.
@Scott Cantor In case you were testing this with a wired builder… I have also renamed the property to requestTimeoutCorePoolSize
, to better align it with how it’s defined, and also made clarifications to the Javadocs there.
Scott Cantor November 12, 2024 at 2:47 PM
I reviewed…my only comment is about the thread pool size. I read what I could find on the behavior, and my impression is that whatever that size is becomes the “core” pool size and that it immediately starts that number of threads up and keeps them running.
That seems high, as in practice we know the actual thread count isn’t likely to be that high most of the time, and it already supports exceeding that pool size up to max int if it needs more.
So my guess is we’d want it a lot lower, maybe 10-20? Obviously would depend on load and we can add a new default property to the parent bean(s) to control it via services.properties.
Brent Putman November 7, 2024 at 11:49 PM
This caused a check in another test to fail b/c it assumed the “outer” client instance produced by the builder was the the context handling one, which is no longer true.
Maybe we expose the wrapped client via a protected
method so tests can introspect.
Brent Putman November 7, 2024 at 11:21 PM
Initial impl checked in as 1008004b231d8159778e747d29bcde09c0016182.
I got fancy and did unit tests with an embedded Jetty that I cribbed from Marvin’s CAS tests. Also tested against a real local httpd server I manage.
This approach seems to work pretty well so far.
On timeout I did make it produce an exception type defined by our library rather than the more generic one that is thrown by HC.
A couple of points/questions:
First, because the code is so simple, I didn’t so far bother implementing a conditional in the builder to suppress the construction of this new type of client. Without an effective timeout specified (in builder or in request context), it pretty much doesn’t do anything. You’ll just have the new instance type passing the execute()
call through to the wrapped client, with the ScheduledExecutorService
that never gets called.
We could add such a conditional in the builder, such as when requestTimeout != null,
etc. But they you’d have to have another boolean to force it on if you wanted to support per-request timeouts via the client context. And if you didn’t turn it on and later did specify a per-request timeout, it wouldn’t get evaled because you wouldn’t have the right type of client. That seems undesirable, and confusing to diagnose/support.
Second, I have initially defaulted the requestTimeoutThreadPoolSize
to 100. We can refine that further if necessary. My thought was it probably should match or exceed the value for maxConnectionsTotal
. But turns out we default that to -1, meaning “not specified”, and then it gets weird. I think internally HC defaults its conn pool to 25, but tracing it is confusing.
Apache
HttpClient
doesn’t have a timeout feature that places an absolute limit on the total end-to-end request/response operation time.Implement such a feature with an async model that uses a
ScheduledExecutorService
to cancel the request if a configured timeout is reached.The implementation will be in the form of a new implementation of the
HttpClient
interface, that wraps a supplied client instance.