Implement HttpClient support for an overall request timeout

Description

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.

Environment

None

Activity

Brent Putman December 6, 2024 at 4:48 AM

Ok I have changed the default pool size to 20.

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.

Details

Assignee

Reporter

Components

Fix versions

Created November 7, 2024 at 10:55 PM
Updated February 12, 2025 at 6:18 PM