shinseongsu commented on PR #923:
URL:
https://github.com/apache/httpcomponents-client/pull/923#issuecomment-6600227573
Love this cleanup. Having all the defaults in one place is going to make
things so much easier to reason about 🙌
On @arturobernalg's question, I went down the rabbit hole a bit:
Classic doesn't actually change. `SocketConfig.DEFAULT` already ships with a
3 minute soTimeout, so `ConfigDefaults.SOCKET` is identical to it. The only
real behavior change in the whole PR is `IO_REACTOR` (0 → 3 min on the async
side). I double-checked on a build of this branch by comparing every constant
with its `XxxConfig.DEFAULT`: six of them are the very same instance, `SOCKET`
has equal values, and `IO_REACTOR` is the one that differs.
Where it gets interesting is the new SSE client, since event streams can
legitimately stay quiet for a long time. I spun up a local server that sends
one event and then goes silent (no heartbeat), with `SseExecutor.newInstance()`
on the client side:
```
before this PR:
[ 0.3s] onOpen
[ 0.3s] onEvent id=1 data=hello
[240.3s] done waiting (240 s)
with this PR:
[ 0.3s] onOpen
[ 0.3s] onEvent id=1 data=hello
[180.5s] onClosed
```
No `onFailure`, no reconnect. The stream just quietly dies at exactly 3
minutes, and that's the scary part.
Looks like SSE has an issue here on its own: the timeout surfaces as a
`SocketTimeoutException`, and `DefaultEventSource.isBenignCancel()` treats any
`InterruptedIOException` as a user cancel, so it goes straight to `onClosed()`.
So anyone who sets a socket timeout today already gets the same silent close;
this PR just makes it the default. Same thing with a 5s timeout on the current
code, (a) being a plain async request and (b) an event source:
```
[ 5.3s] (a) failed java.net.SocketTimeoutException: 5 SECONDS | instanceof
InterruptedIOException=true
[ 5.3s] (b) onOpen
[ 5.3s] (b) onEvent id=1
[10.3s] (b) onClosed
```
For what it's worth, I totally agree 3 minutes is a much saner default than
"wait forever" for regular request/response traffic. OkHttp made the same call
back in 2.5 ("that was a lousy policy" 😄). But since it's a real behavior
change, maybe keep `IO_REACTOR` as is here and land the timeout change
separately with a release note? Happy to put up a PR for the SSE side so a read
timeout triggers a reconnect instead of a silent close.
One more small thing: the new default only kicks in when no
`IOReactorConfig` is passed.
`IOReactorConfig.custom().setIoThreadCount(4).build()` still ends up with
soTimeout=0, so tweaking an unrelated setting quietly turns the timeout off
again.
Nits:
- `WebSocketClientBuilder.build()` still uses `Http1Config.DEFAULT` /
`H2Config.DEFAULT`
- `WebSocketServerConnectionFactory` mixes both styles (and it's a server
class pulling in the client defaults)
- the examples now reference an `@Internal` class, so `TlsConfig.DEFAULT`
might read nicer there
Thanks for the ping, really nice cleanup overall!
--
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]
