shinseongsu commented on PR #926:
URL:
https://github.com/apache/httpcomponents-client/pull/926#issuecomment-6629590864
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.
--
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]
