Enforce Java 11 response body timeouts

[goutamadwant]

Summary

This updates the Java 11 Http2Client response handling so the configured Feign read timeout also applies while the response body is being read.

The existing request timeout still covers waiting for the response headers, but BodyHandlers.ofInputStream() can hand back a response before the body has arrived. The response body stream is now wrapped with a read-timeout guard, preserving the existing streaming response behavior while failing stalled body reads with HttpTimeoutException.

Tests

• mvn --batch-mode -Dtoolchain.skip=true -pl java11 -am -Dtest=Http2ClientTest#timeoutReadingResponseBody,Http2ClientAsyncTest#timeoutReadingResponseBody -Dsurefire.failIfNoSpecifiedTests=false test
• mvn --batch-mode -Dtoolchain.skip=true -pl java11 -am -Dtest=Http2ClientTest,Http2ClientAsyncTest -Dsurefire.failIfNoSpecifiedTests=false test
• mvn --batch-mode -Dtoolchain.skip=true -pl java11 -am -Dsurefire.failIfNoSpecifiedTests=false test
• mvn --batch-mode -Dtoolchain.skip=true -pl java11 -am -DskipTests validate
• mvn --batch-mode -Dtoolchain.skip=true -pl java11 -am git-code-format:validate-code-format

Fixes #3068

[velo]

This feels like it belong on a interceptor or capability.... it is an orthogonal feature that we could apply to all clients.

[velo]

Good problem to solve — the Java 11 client genuinely ignores readTimeout once headers arrive, and I like that you covered both the sync and async paths with tests. Two things need fixing before this can go in.

1. Subclass overrides of toFeignResponse are silently bypassed

execute() now calls the new private three-arg toFeignResponse(Request, HttpResponse, Options). The existing protected Response toFeignResponse(Request, HttpResponse) is left in place but is no longer on the execution path — so any subclass overriding it (a documented extension point today) keeps compiling and keeps passing its own tests, while its code stops being called in production. That's the worst kind of breaking change: invisible.

Http2ClientContentLengthTest hides this, because it calls the two-arg form directly and therefore exercises a path production no longer uses.

Please make the three-arg overload protected and reduce the two-arg one to a delegating shim (return toFeignResponse(request, httpResponse, null);), so overriding either signature still works.

2. One scheduled task per read() call

readWithTimeout schedules a ScheduledFuture and cancels it on every invocation, including the single-byte read(). For a consumer reading unbuffered, that's a schedule+cancel round-trip on a shared single-threaded executor per byte. Two allocations of AtomicBoolean per byte on top.

Schedule one deadline for the stream (or at least per read(byte[], int, int) batch) and check remaining time, rather than arming a fresh timer per read.

Smaller notes

• TimeoutInputStream doesn't delegate skip, mark, reset, markSupported. InputStream#skip routes through read, so it's correct, but markSupported silently degrades to false for a delegate that supported it.
• BODY_READ_TIMEOUT_EXECUTOR is a process-wide static that's never shut down. Daemon thread makes that survivable, but it means every Http2Client in the JVM shares one timer thread — worth a comment at minimum.
• In readWithTimeout, the throw timeoutException == null ? timeoutException(e) : timeoutException; line reads awkwardly. if (timeoutException != null) throw timeoutException; throw timeoutException(e); is clearer.
• Please make sure the new tests use Retryer.NEVER_RETRY consistently — the async one doesn't set it, so a retry would multiply the 1s body delay.

[goutamadwant]

Thanks for your review @velo. Addressed all your comments and suggestions. Can you pls take a look again at the changes? thanks!

[velo]

Thanks for picking this up — #3068 is a real gap and the guard-stream idea is a reasonable way in. But I have two design-level objections plus a handful of concrete issues before this can go in.

1. This silently redefines what readTimeout means. Everywhere else in Feign — including Client.Default, where it maps to SO_TIMEOUT — readTimeout is an idle timeout: time between reads. Here it becomes a hard deadline on the entire body transfer. A 30s readTimeout now kills a perfectly healthy 31s download, and streaming responses (users returning feign.Response and consuming body().asInputStream() themselves) stop working with any finite readTimeout.

I know the issue suggests orTimeout, which has the same semantics, and explicitly calls the correct fix (a custom body subscriber) non-trivial. I'm not necessarily against shipping the deadline version — but it has to be a deliberate, documented decision, not a side effect. At minimum: javadoc on the behaviour, a note in the changelog, and ideally a way to opt out.

2. The clock starts at the wrong moment. The timer is armed when the HttpResponse object is constructed, not when the caller starts reading. In the async path thenApply(...) runs on the HttpClient executor, so the response can sit through the decoder/retryer before a single byte is read. Under load you get spurious timeouts on responses that never stalled — the opposite of what the option is for.

Inline comments cover the rest: an unbounded cancelled-task queue on a shared static scheduler, and a restructuring that would delete ~90 lines of this PR.

Marking this as a comment rather than request-changes, but points 1, 2 and the setRemoveOnCancelPolicy one all need resolving before merge.

[velo]

Two problems with this executor.

Cancelled tasks are never removed from the queue. ScheduledThreadPoolExecutor defaults removeOnCancelPolicy to false, so a task cancelled in close()/at EOF stays in the DelayedWorkQueue until its original deadline elapses. Every response arms one of these, and the happy path cancels essentially all of them. At 1k req/s with a 60s readTimeout that's ~60k dead queue nodes resident at steady state, each holding a strong reference to a response InputStream and its buffers. This is the one I'd consider a blocker.

Fix: build it as a ScheduledThreadPoolExecutor and call setRemoveOnCancelPolicy(true).

Static, never shut down. The thread is created lazily on first schedule(), inheriting the context classloader of whichever caller happens to trigger it — the classic webapp/OSGi classloader pin on undeploy. Worth at least making it lazily initialised behind a holder so the class doesn't own a live thread for the lifetime of the JVM.

(CompletableFuture.delayedExecutor would avoid creating a thread at all, but note it gives you no handle to cancel with, so you'd trade the queue-retention problem for a different shape of it. The removeOnCancelPolicy fix is the simpler one.)

[velo]

This new protected overload, TimeoutHttpResponse below, and the two *ArgumentOverrideClient tests all exist for one reason: you're wrapping the JDK HttpResponse so the existing 2-arg toFeignResponse can be reused unchanged.

Wrap the Feign Response instead and all of it goes away:

Response response = toFeignResponse(request, httpResponse);
return withReadTimeout(response, options);
// withReadTimeout: response.toBuilder().body(wrapped, response.body().length())

That's no new protected API to maintain, no 60-line HttpResponse mirror, no tests needed to prove the overload plumbing dispatches correctly — and as a bonus the guard sits outside the gzip/inflate layer, so it fires on the outermost read rather than on the compressed bytes.

Also: this is the point where the deadline is armed, which is my objection (2) in the review body — for the async path this runs on the HttpClient executor, potentially well before the caller reads anything.

[velo]

For the record, the semantics encoded here: a single schedule() at construction that fires unconditionally, regardless of whether data has been flowing. That's a total-transfer deadline, not a read timeout.

If you want to keep the class but get idle-timeout semantics, the shape is a re-armed deadline: track lastReadNanos, and have the scheduled task compare against it and reschedule itself for the remainder instead of closing when the stream has made progress since it was armed. Costs one volatile write per read and behaves the way SO_TIMEOUT does everywhere else in Feign.

[velo]

Nit: if the timer fires between delegate.read(b, off, len) returning n and checkTimedOut(), those n bytes are already in the caller's buffer but the call throws and the count is lost. Practically harmless since it's a terminal timeout, but read isn't exception-safe as written — checking the flag before delegating would be.

[velo]

available(), mark(), reset() and markSupported() all bypass the timedOut check. After the timer has closed the delegate, available() will either return 0 or throw a raw IOException depending on the underlying stream, rather than the HttpTimeoutException the rest of the class is careful to produce. reset() in particular can hand back a stream that then times out mid-read.

[velo]

60 lines of pure delegation to swap out one method. This whole class disappears if the wrapping moves to the Feign Response — see my comment on the 3-arg toFeignResponse above.

[velo]

A 1ms timeout plus a 5s latch await is asking for trouble on CI — this asserts on scheduler latency, not on behaviour. Http2ClientTest is already one of our flakier suites and I'd rather not add to it.

If the timeout wrapper takes its scheduler (or a clock) as a seam rather than reaching for a static field, this becomes a deterministic test with no sleeping at all.

[velo]

Please use an import rather than the fully-qualified java.net.http.HttpResponse here and in ThreeArgumentOverrideClient — house style throughout the repo.

That said, both of these test doubles only exist to verify that a protected overload dispatches to its sibling. If the wrapping moves onto the Feign Response (see Http2Client.java), the overload and both of these classes are unnecessary.