[ok2c]

@arturobernalg Please double-check.

[ok2c]

CC @shinseongsu

[arturobernalg]

Looks good overall.

One concern: ConfigDefaults.IO_REACTOR changes the default socket timeout from 0 to 3 minutes, including for WebSocket and minimal async clients.

Is this intentional? It changes existing behavior rather than just consolidating defaults.

[shinseongsu]

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!

[ok2c]

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.

@shinseongsu Damn. I completely overlooked that. We will have to change IOReactorConfig if we want to do it right. Thank you for pointing it out.

Now, what shall I do about this change-set? Does it still make any sense? Is there any value in having one class with config defaults if all it does is referencing the #DEFAULT objects?

@shinseongsu @arturobernalg What do you think?

[arturobernalg]

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.

@shinseongsu Damn. I completely overlooked that. We will have to change IOReactorConfig if we want to do it right. Thank you for pointing it out.

Now, what shall I do about this change-set? Does it still make any sense? Is there any value in having one class with config defaults if all it does is referencing the #DEFAULT objects?

@shinseongsu @arturobernalg What do you think?

@ok2c I would drop it. If ConfigDefaults only aliases the existing defaults, I don’t think the extra indirection adds much value.

The async socket timeout is a separate behavioral change and should be handled independently.