arturobernalg commented on PR #926:
URL:
https://github.com/apache/httpcomponents-client/pull/926#issuecomment-6631606812
> Thanks for picking this up so quickly! I ran this branch locally: HTTP/1.1
now behaves as intended (server sends one event and goes silent,
responseTimeout=3s):
>
> ```
> master: [ 3.3s] onClosed
> this PR: [ 3.3s] onFailure SocketTimeoutException: 3 SECONDS |
willReconnect=true
> [ 3.8s] onOpen (reconnects every ~3 s)
> ```
>
> Over HTTP/2 it still closes silently, though. H2 exchanges get the
response timeout per stream, and when it fires the stream is reset with
`H2StreamTimeoutException`, which extends `InterruptedIOException` but not
`SocketTimeoutException`. Same server, `customHttp2()` client:
>
> ```
> this PR: [ 5.3s] onClosed
> (debug: ex-0000000001: execution failed: HTTP/2 stream idle
timeout (3 SECONDS))
> ```
>
> Dropping `InterruptedIOException` from the benign list isn't an option,
since the async client signals cancellation with a plain `new
InterruptedIOException()` (e.g. `HttpAsyncMainClientExec#cancel`). So I'd check
for timeouts explicitly before the benign check:
>
> ```java
> private static boolean isBenignCancel(final Exception ex) {
> if (isTimeout(ex)) {
> return false;
> }
> return ex instanceof RequestNotExecutedException
> || ex instanceof ConnectionClosedException
> || ex instanceof CancellationException
> || ex instanceof InterruptedIOException;
> }
>
> private static boolean isTimeout(final Exception ex) {
> return ex instanceof SocketTimeoutException
> || ex instanceof H2StreamTimeoutException;
> }
> ```
>
> With that, the H2 case reports and reconnects:
>
> ```
> [ 5.3s] onFailure H2StreamTimeoutException: HTTP/2 stream idle
timeout (3 SECONDS) | willReconnect=true
> [ 5.6s] onOpen
> ```
>
> I also turned the socket timeout test into a `@ParameterizedTest` over
`SocketTimeoutException`, `ConnectTimeoutException` and
`H2StreamTimeoutException` (the H2 case fails on this branch, all pass with the
change; httpclient5-sse verify: 55 tests, 0 failures). Happy to push it to your
branch or open a follow-up, whichever you prefer.
>
> Side note: since `ConnectTimeoutException` extends
`SocketTimeoutException`, connect timeouts now reconnect too instead of closing
silently. Probably worth a line in the commit message.
@shinseongsu please do another pass
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
