Skip to content
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.

Sign up for GitHub

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

Optimize connection close logic to resolve timeout delay issue #508

Merged
merged 7 commits into from
Nov 28, 2024
Merged

Optimize connection close logic to resolve timeout delay issue #508

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f87a81b
Add option to close connections with RST on timeout
ooeunz Nov 27, 2024
c66648c
Add version tag
ooeunz Nov 27, 2024
0c2c722
Change default behavior to IMMEDIATE close
ooeunz Nov 27, 2024
1b2709a
Separate connection close handling for IOException and regular except…
ooeunz Nov 27, 2024
4bd75b9
Use closeQuietly for SSLException handling
ooeunz Nov 27, 2024
7247895
Fix code style
ooeunz Nov 27, 2024
d243833
Replace wildcard imports with explicit imports
ooeunz Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions httpcore5/src/main/java/org/apache/hc/core5/http/config/Http1Config.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,11 @@ public class Http1Config {
private final int maxHeaderCount;
private final int maxEmptyLineCount;
private final int initialWindowSize;
private final boolean useRstOnTimeout;

Http1Config(final HttpVersion version, final int bufferSize, final int chunkSizeHint,
final Timeout waitForContinueTimeout, final int maxLineLength, final int maxHeaderCount,
final int maxEmptyLineCount, final int initialWindowSize) {
final int maxEmptyLineCount, final int initialWindowSize, final boolean useRstOnTimeout) {
super();
this.version = version;
this.bufferSize = bufferSize;
Expand All @@ -69,6 +70,7 @@ public class Http1Config {
this.maxHeaderCount = maxHeaderCount;
this.maxEmptyLineCount = maxEmptyLineCount;
this.initialWindowSize = initialWindowSize;
this.useRstOnTimeout = useRstOnTimeout;
}

/**
Expand Down Expand Up @@ -109,6 +111,13 @@ public int getInitialWindowSize() {
return initialWindowSize;
}

/**
* @since 5.4
*/
public boolean getUseRstOnTimeout() {
Copy link
Member

Choose a reason for hiding this comment

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

@ooeunz Please add @since 5.4 tag

return useRstOnTimeout;
}

@Override
public String toString() {
final StringBuilder builder = new StringBuilder();
Expand All @@ -120,6 +129,7 @@ public String toString() {
.append(", maxHeaderCount=").append(maxHeaderCount)
.append(", maxEmptyLineCount=").append(maxEmptyLineCount)
.append(", initialWindowSize=").append(initialWindowSize)
.append(", useRstOnTimeout=").append(useRstOnTimeout)
.append("]");
return builder.toString();
}
Expand Down Expand Up @@ -147,6 +157,7 @@ public static Http1Config.Builder copy(final Http1Config config) {
private static final int INIT_MAX_HEADER_COUNT = -1;
private static final int INIT_MAX_LINE_LENGTH = -1;
private static final int INIT_MAX_EMPTY_LINE_COUNT = 10;
private static final boolean USE_RST_ON_TIMEOUT = false;

public static class Builder {

Expand All @@ -158,6 +169,7 @@ public static class Builder {
private int maxHeaderCount;
private int maxEmptyLineCount;
private int initialWindowSize;
private boolean userRstOnTimeout;

Builder() {
this.version = HttpVersion.HTTP_1_1;
Expand All @@ -168,6 +180,7 @@ public static class Builder {
this.maxHeaderCount = INIT_MAX_HEADER_COUNT;
this.maxEmptyLineCount = INIT_MAX_EMPTY_LINE_COUNT;
this.initialWindowSize = INIT_WINDOW_SIZE;
this.userRstOnTimeout = USE_RST_ON_TIMEOUT;
}

/**
Expand Down Expand Up @@ -222,6 +235,14 @@ public Builder setInitialWindowSize(final int initialWindowSize) {
return this;
}

/**
* @since 5.4
Copy link
Member

Choose a reason for hiding this comment

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

I think you need a real Javadoc description that explains what this toggle does.

*/
public Builder setUserRstOnTimeout(final boolean userRstOnTimeout) {
Copy link
Member

Choose a reason for hiding this comment

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

@ooeunz Please add @since 5.4 tag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. Thanks.

this.userRstOnTimeout = userRstOnTimeout;
return this;
}

public Http1Config build() {
return new Http1Config(
version,
Expand All @@ -231,7 +252,8 @@ public Http1Config build() {
maxLineLength,
maxHeaderCount,
maxEmptyLineCount,
initialWindowSize);
initialWindowSize,
userRstOnTimeout);
}

}
Expand Down
7 changes: 6 additions & 1 deletion httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/HttpRequestExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
import org.apache.hc.core5.http.protocol.HttpContext;
import org.apache.hc.core5.http.protocol.HttpCoreContext;
import org.apache.hc.core5.http.protocol.HttpProcessor;
import org.apache.hc.core5.io.CloseMode;
import org.apache.hc.core5.io.Closer;
import org.apache.hc.core5.util.Args;
import org.apache.hc.core5.util.Timeout;
Expand Down Expand Up @@ -212,7 +213,11 @@ public ClassicHttpResponse execute(
return response;

} catch (final HttpException | IOException | RuntimeException ex) {
Closer.closeQuietly(conn);
ok2c marked this conversation as resolved.
Show resolved Hide resolved
if (http1Config.getUseRstOnTimeout()) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a use case for keeping the old behavior?

Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Member

Choose a reason for hiding this comment

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

TY @ok2c for the explanation. I think details like this should be in the Javadoc for the getter-setter pair.

Closer.close(conn, CloseMode.IMMEDIATE);
} else {
Closer.closeQuietly(conn);
}
throw ex;
}
}
Expand Down