-
Notifications
You must be signed in to change notification settings - Fork 983
Add socket timeout integration tests #648
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I was startled to learn that 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. |
httpclient5-testing/src/main/java/org/apache/hc/client5/testing/async/AsyncRandomHandler.java
Show resolved
Hide resolved
httpclient5-testing/src/main/java/org/apache/hc/client5/testing/classic/RandomHandler.java
Show resolved
Hide resolved
httpclient5-testing/src/main/java/org/apache/hc/client5/testing/classic/RandomHandler.java
Outdated
Show resolved
Hide resolved
httpclient5-testing/src/main/java/org/apache/hc/client5/testing/async/AsyncRandomHandler.java
Show resolved
Hide resolved
@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`.
Yeah, this is probably one reason why virtually all server IO is non-blocking, even when the API is synchronous (see for example Tomcat). |
| // TODO: Why does `CloseMode.GRACEFUL` (the test client default) | ||
| // block until the IOReactor shutdown timeout when using UDS? | ||
| client.close(CloseMode.IMMEDIATE); |
There was a problem hiding this comment.
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.
|
@ok2c Anything else on this one? |
ok2c
left a comment
There was a problem hiding this 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
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
/randomrequest 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 ofConnectionConfig,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.