Uh oh!
There was an error while loading. Please reload this page.
Optimize connection close logic to resolve timeout delay issue - #508
Conversation
- Introduced a configuration option `useRstOnTimeout` in `Http1Config` to allow connections to be closed with an RST (Reset) signal when a timeout occurs. - Updated `HttpRequestExecutor` to respect this configuration and invoke the appropriate close mode (`CloseMode.IMMEDIATE` for RST or `Closer.closeQuietly` for FIN). - Maintained backward compatibility by defaulting to FIN-based connection closure. This change improves flexibility and allows faster resource cleanup in timeout scenarios, aligning with the HTTP/1.1 recommendations for abnormal connection termination. Resolves: HTTPCLIENT-2324
| return initialWindowSize; | ||
| } | ||
| public boolean getUseRstOnTimeout() { |
| return this; | ||
| } | ||
| public Builder setUserRstOnTimeout(final boolean userRstOnTimeout) { |
| } | ||
| /** | ||
| * @since 5.4 |
There was a problem hiding this comment.
I think you need a real Javadoc description that explains what this toggle does.
| } catch (final HttpException | IOException | RuntimeException ex) { | ||
| Closer.closeQuietly(conn); | ||
| if (http1Config.getUseRstOnTimeout()) { |
There was a problem hiding this comment.
Is there a use case for keeping the old behavior?
There was a problem hiding this comment.
@garydgregory Graceful shutdown of TLS connections would be one. One can drop a TLS connection without a close-notify handshake but it is generally considered rude.
@ooeunz Why do not we always use IMMEDIATE close for plain (non TLS) conections by default? Would that make the new config flag redundant?
There was a problem hiding this comment.
To maintain backward compatibility, the existing behavior has been preserved.
However, if it is determined that there are no issues with backward compatibility, I would like to make immediate close the default behavior.
There was a problem hiding this comment.
I think that a socket timeout indicates that the server is not functioning properly, so setting immediate close as the default behavior should not cause any issues.
There was a problem hiding this comment.
TY @ok2c for the explanation. I think details like this should be in the Javadoc for the getter-setter pair.
Uh oh!
There was an error while loading. Please reload this page.
ok2c
commented
Nov 27, 2024
@ooeunz Almost there. Please fix the style check violations and update the title of PR and will merge the change-set. |
ooeunz
commented
Nov 28, 2024
@ok2c@garydgregory I’ve completed refining the code style. It was such an enjoyable experience to collaborate and build the code together! 😊 |
ok2c
commented
Nov 28, 2024
@ooeunz Cherry-picked to |
Description
This change improves flexibility and allows faster resource cleanup in timeout scenarios, aligning with the HTTP/1.1 recommendations for abnormal connection termination.
Problem Description
There is an issue where the connection close operation takes as long as the SocketTimeout value set in the SocketConfig after a Socket timeout occurs. This behavior arises because the NioSocketImpl performs an additional poll for the duration of the SocketTimeout during the close operation. Consequently, HTTP requests may experience delays of up to twice the configured SocketTimeout value.
Proposed Solution
This Pull Request introduces the useRstOnTimeout configuration option to mitigate this issue by enabling connections to be closed using an RST signal instead of a FIN-based closure.
The following image shows the test results with the socket timeout value set to 5 seconds
Example Workflow Before Fix
The socket reaches the timeout value specified in the SocketTimeout configuration.
During the close operation, the NioSocketImpl performs an additional poll for the same timeout duration.
HTTP requests may end up waiting for up to twice the configured SocketTimeout value.
Resolves: HTTPCLIENT-2324