Skip to content

Conversation

@rschmitt
Copy link
Contributor

@rschmitt rschmitt commented Jun 9, 2025

This change adds integration test coverage for various types of read timeouts. The test coverage includes HTTP, HTTPS, and HTTP over UDS if available. The /random request handlers have been augmented with delay support, so that the clients can read the response headers and then time out while reading the entity. The tests make use of ConnectionConfig, ResponseTimeout, and (on the sync client only) SocketConfig, and demonstrate the precedence among these config options when more than one is supplied.

One issue uncovered by these tests is that graceful closure of the async client does not work when UDS socket timeouts have fired. To work around this until the issue can be root caused, the async UDS tests use CloseMode.IMMEDIATE.

@rschmitt
Copy link
Contributor Author

rschmitt commented Jun 9, 2025

I was startled to learn that java.net.Socket::setSoTimeout only sets read timeouts:

https://github.com/openjdk/jdk/blob/d186dacdb7b91dc9a28b703ce3c8ea007fc450b6/src/java.base/share/classes/java/net/Socket.java#L1258-L1260

Given that this is the case, I'm not sure what prevents synchronous clients from going to sleep forever if a write blocks indefinitely because the send buffer filled up. Additionally, I haven't found a way to simulate this condition using localhost (at least not on macOS), so I can't even determine whether there's a sensible default value or something.

@ok2c
Copy link
Member

ok2c commented Jun 10, 2025

I was startled to learn that java.net.Socket::setSoTimeout only sets read timeouts:

https://github.com/openjdk/jdk/blob/d186dacdb7b91dc9a28b703ce3c8ea007fc450b6/src/java.base/share/classes/java/net/Socket.java#L1258-L1260

Given that this is the case, I'm not sure what prevents synchronous clients from going to sleep forever if a write blocks indefinitely because the send buffer filled up. Additionally, I haven't found a way to simulate this condition using localhost (at least not on macOS), so I can't even determine whether there's a sensible default value or something.

@rschmitt In the older version of HttpClient the socket timeout used to be called SO_READ_TIMEOUT. This is yet another limitation of the classic i/o one must tolerate

This change adds integration test coverage for various types of read
timeouts. The test coverage includes HTTP, HTTPS, and HTTP over UDS if
available. The `/random` request handlers have been augmented with delay
support, so that the clients can read the response headers and then time
out while reading the entity. The tests make use of `ConnectionConfig`,
`ResponseTimeout`, and (on the sync client only) `SocketConfig`, and
demonstrate the precedence among these config options when more than one
is supplied.

One issue uncovered by these tests is that graceful closure of the async
client does not work when UDS socket timeouts have fired. To work around
this until the issue can be root caused, the async UDS tests use
`CloseMode.IMMEDIATE`.
@rschmitt
Copy link
Contributor Author

This is yet another limitation of the classic i/o one must tolerate.

Yeah, this is probably one reason why virtually all server IO is non-blocking, even when the API is synchronous (see for example Tomcat).

Comment on lines +160 to +162
// TODO: Why does `CloseMode.GRACEFUL` (the test client default)
// block until the IOReactor shutdown timeout when using UDS?
client.close(CloseMode.IMMEDIATE);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to look into this some more. Based on the logspam I get during IOReactor shutdown, it's some sort of low-level selector issue. Could be a behavioral difference with UDS SocketChannels.

@rschmitt
Copy link
Contributor Author

@ok2c Anything else on this one?

Copy link
Member

@ok2c ok2c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rschmitt Not really. Looks good to me

@rschmitt rschmitt merged commit 06481a0 into apache:master Jun 16, 2025
10 checks passed
@rschmitt rschmitt deleted the so-timeout branch June 16, 2025 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants