IImproved cancellability of HTTP message streams / response timeout support#814
IImproved cancellability of HTTP message streams / response timeout support#814ok2c wants to merge 1 commit intoapache:masterfrom
Conversation
* Improved response timeout support
rschmitt
left a comment
rschmitt
left a comment
There was a problem hiding this comment.
it makes it possible for the full-featured async HttpClient to abort individual messages exchanges over HTTP/2 connections without shutting down the connection
This sounds like it would merit a test case or two:
- Cancellation of a message exchange leaves the h2 connection open
- Cancellation of one message exchange does not result in the termination of other open message exchanges on the same h2 connection
Fixes response time implementation for message exchanges over HTTP/1.1 and HTTP/2 connections
We should add h2 and maybe h2c coverage to TestAsyncSocketTimeout. I tried it just now, but client.getConnectionManager() returns null, which breaks two out of three test cases. The third test case fails due to the timeout not firing, even with this PR applied, which seems wrong. You can try it by adding this to TestAsyncSocketTimeout:
@Nested
class H2 extends AbstractTestSocketTimeout {
public H2() {
super(URIScheme.HTTPS, ClientProtocolLevel.H2_ONLY, ServerProtocolLevel.H2_ONLY, false);
}
}| final Timeout responseTimeout = requestConfig.getResponseTimeout(); | ||
| final EndpointInfo endpointInfo = endpoint.getInfo(); | ||
| final ProtocolVersion version = endpointInfo != null ? endpointInfo.getProtocol() : null; | ||
| final boolean supportsStreams = version != null && version.greaterEquals(HttpVersion.HTTP_2); |
There was a problem hiding this comment.
I suggest supportsMultiplexing or just isH2. "Stream" is an overloaded term and to me mainly connotes InputStreams and the like, i.e. streaming in contrast to buffering.
| return new ReleasingAsyncClientExchangeHandler(handler, this::releaseSlot); | ||
| } | ||
|
|
||
| private Cancellable doExecute( |
There was a problem hiding this comment.
Ideally, preparatory refactorings like this method extraction would be done in separate commits for ease of review (they can still be submitted in a single PR and squashed before merging)
|
@rschmitt Okay. I will split the change-set in a number of smaller ones to make it easier to review. I will also try to put together some integrations tests for message exchange cancellation but request cancellation was notoriously difficult to build reliable tests for. We still have a number of disabled ones that never got fixed because they never fail locally but fail occasionally in CI. |
2 participants
@arturobernalg @rschmitt Please take a look at this change-set and let me know if you see anything wrong or anything disagreeable. The change-set improves in two areas: