Add the ability to configure SocketOptions on the AwsCrtAsyncHttpClient #3457

nikp · 2022-10-03T21:47:33Z

Motivation and Context

This is necessary for any clients with long-running connections that exceed default socket timeouts of services along the call path, and need to enable keep-alive settings which the CRT client supports, but the Java client wasn't exposing to callers

Modifications

Adds a new configuration parameter object to encapsulate SocketOptions to the AwsCrtAsyncHttpClient DefaultBuilder

Testing

Unit testing - the change is very isolated from the rest of the package

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
[] Local run of mvn install succeeds
My mvn install does not succeed due to [ERROR] Failed to execute goal org.apache.maven.plugins:maven-shade-plugin:3.1.0:shade (default) on project third-party-jackson-core: Error creating shaded jar: duplicate entry: META-INF/services/software.amazon.awssdk.thirdparty.jackson.core.JsonFactory, however the code I added is independent and I verified that the new code unit tests pass
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

^ I'm actually not sure about the last one - @JonathanHenson guided us to this change but I do see the LaunchChangelog references TCP Keep-Alive. Is it related?

License

I confirm that this pull request can be released under the Apache 2 license

JonathanHenson

LGTM!, someone from the Java SDK team could you have a look?

...aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/SocketOptionsConfiguration.java

L-Applin · 2022-10-04T16:13:06Z

Just noting that this PR would also resolve resolve #3251, and make #3456 obsolete as this supersedes it.

...ents/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/AwsCrtAsyncHttpClient.java

zoewangg · 2022-10-04T17:30:49Z

...aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/SocketOptionsConfiguration.java

+        private int connectTimeoutMs = 3000;
+        private int keepAliveIntervalSecs = 0;
+        private int keepAliveTimeoutSecs = 0;


Instead of defining defaults here, I think we should put the configured values into the standard options map like below. The SDK will use the global defaults automatically for any options that are not configured

aws-sdk-java-v2/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/NettyNioAsyncHttpClient.java

Lines 542 to 545 in b1f8989

public Builder connectionTimeout(Duration timeout) {

Validate.isPositive(timeout, "connectionTimeout");

standardOptions.put(SdkHttpConfigurationOption.CONNECTION_TIMEOUT, timeout);

return this;

aws-sdk-java-v2/http-client-spi/src/main/java/software/amazon/awssdk/http/SdkHttpConfigurationOption.java

Lines 155 to 171 in b1f8989

public static final AttributeMap GLOBAL_HTTP_DEFAULTS = AttributeMap

.builder()

.put(READ_TIMEOUT, DEFAULT_SOCKET_READ_TIMEOUT)

.put(WRITE_TIMEOUT, DEFAULT_SOCKET_WRITE_TIMEOUT)

.put(CONNECTION_TIMEOUT, DEFAULT_CONNECTION_TIMEOUT)

.put(CONNECTION_ACQUIRE_TIMEOUT, DEFAULT_CONNECTION_ACQUIRE_TIMEOUT)

.put(CONNECTION_MAX_IDLE_TIMEOUT, DEFAULT_CONNECTION_MAX_IDLE_TIMEOUT)

.put(CONNECTION_TIME_TO_LIVE, DEFAULT_CONNECTION_TIME_TO_LIVE)

.put(MAX_CONNECTIONS, DEFAULT_MAX_CONNECTIONS)

.put(MAX_PENDING_CONNECTION_ACQUIRES, DEFAULT_MAX_CONNECTION_ACQUIRES)

.put(PROTOCOL, DEFAULT_PROTOCOL)

.put(TRUST_ALL_CERTIFICATES, DEFAULT_TRUST_ALL_CERTIFICATES)

.put(REAP_IDLE_CONNECTIONS, DEFAULT_REAP_IDLE_CONNECTIONS)

.put(TCP_KEEPALIVE, DEFAULT_TCP_KEEPALIVE)

.put(TLS_KEY_MANAGERS_PROVIDER, DEFAULT_TLS_KEY_MANAGERS_PROVIDER)

.put(TLS_TRUST_MANAGERS_PROVIDER, DEFAULT_TLS_TRUST_MANAGERS_PROVIDER)

.put(TLS_NEGOTIATION_TIMEOUT, DEFAULT_TLS_NEGOTIATION_TIMEOUT)

Can move those there, sure.

Based on the discussion in #3457 (comment), i think it doesn't make sense to list defaults for these

zoewangg · 2022-10-04T17:31:34Z

...aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/SocketOptionsConfiguration.java

+         * @param domain socket domain
+         * @return Builder
+         */
+        Builder domain(SocketOptions.SocketDomain domain);


Do we need domain and type? If I understand correctly, there's no use-case for configuring these two with AWS services, right? We can always add them in the future if needed.

They probably need to be kept for later http/3, and other alternative transports, but yes assuming this is for building an http client, it's currently always TCP

I ended up keeping them for the advanced users, since it's all going to the same place in the CRT, but decoupled from both connection timeouts, and the keep-alive configuration so that SDK users discovering this can get their iteratively

IMO we should only expose public APIs if needed to keep it a 2-way door because it'd be a lot harder to remove/change them in the future.

Are you saying I should remove SocketOptionsConfiguration from this change so we can decide later how to expose them? I'm fine with that

zoewangg · 2022-10-04T17:41:01Z

...aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/SocketOptionsConfiguration.java

+        Builder keepAliveIntervalSecs(int keepAliveIntervalSecs);
+
+        /**
+         * Sets the number of seconds to wait for a keepalive response before considering the connection timed out
+         * @param keepAliveTimeoutSecs number of seconds to wait for a keepalive response before considering the connection timed out
+         * @return Builder
+         */
+        Builder keepAliveTimeoutSecs(int keepAliveTimeoutSecs);


Do we expect customers to configure keepAliveIntervalSecs and keepAliveTimeoutSecs? If not, should we just introduce tcpKeepAlive(Boolean tcpKeepAlive) and pass default values defined in the SDK to CRT? Because exposing too many knobs may make the SDK more complex to use.

Will default to your judgement there. I suspect just being able to enable keepAlive should be enough.

no, keep alive is worthless without setting those values. The kernel defaults are never what anyone needs for these use-cases. So, it needs to be here.

My concern is that these settings may be too "advanced", so some customers may just set some random values w/o doing any research. I'm fine with adding these two, but I think we should still introduce a boolean tcpKeepAlive and define some "safe" defaults for tcpInterval and tcpKeepAliveIntervalSecs if they are not configured. WDYT?

IMO I think the right way to do this would be to separate KeepAliveConfiguration into a dedicated object.

If it's not set, that means customers want the default, disabled. If they DO set it, then they should be setting both the interval and the timeout, and they're "opting-in" to knowing what they're doing.

It's either add these values or don't bother. The kernel defaults will not meet anyone's needs. It's on the scale of hours before the first probe gets sent. It IS too advanced, but the answer isn't to do it for the user. By the time you're tuning keep alive, both of these values need to be a conscious decision by the user.

@nikp I think a separate class is fine. Can we name it TcpKeepAliveConfiguration though? because people may confuse it with HTTP keep alive (we've had customers asking for clarification before)

@JonathanHenson I think that's fair. We'd probably need to provide some guidance on how to tune them.

I'm gonna submit a new revision with that approach - I think it'll work nicely :)

I submitted TcpKeepAliveConfiguration, please take a look @zoewangg

nikp · 2022-10-04T17:57:25Z

OK I think I can address the requested comments but there are 3 questions for @zoewangg in my mind that are not clear and worth leaving a record of for future extension.

@L-Applin added connection timeout setting. I added keep-alive setting. Should they be top-level fields on AwsCrtAsyncHttpClient.Builder (peers to connectionMaxIdleTime), or should they be in a sub-configuration object.
Connection timeout already has a default in SdkHttpConfigurationOption of 2 seconds, however the underlying SocketOptions that sets the CRT options defaults to 3. Since it looks like today the DEFAULT_CONNECTION_TIMEOUT configuration isn't being applied, that means the effective default is 3 seconds. What would be considered the safe non-breaking way to resolve this inconsistency?
You mentioned that NettyNioAsyncHttpClient already supports configuration of both connectionTimeout and enableTcpKeepAlive and that the CRT client could share a common interface or base calss. Do you want to extract this interface as part of this CR (I assume to SdkAsyncHttpClient/SdkAsyncHttpClient.Builder, or leave it for a future change?

zoewangg · 2022-10-04T18:24:03Z

@L-Applin added connection timeout setting. I added keep-alive setting. Should they be top-level fields on AwsCrtAsyncHttpClient.Builder (peers to connectionMaxIdleTime), or should they be in a sub-configuration object.

I think they should be top-level fields. To be clear, we are going to just add connectionTimeout(Duration connectionTimeout) and tcpKeepAlive(Boolean tcpKeepAlive) right?

Connection timeout already has a default in SdkHttpConfigurationOption of 2 seconds, however the underlying SocketOptions that sets the CRT options defaults to 3. Since it looks like today the DEFAULT_CONNECTION_TIMEOUT configuration isn't being applied, that means the effective default is 3 seconds. What would be considered the safe non-breaking way to resolve this inconsistency?

I think this should be fine. CRT HTTP Client is still in Developer Preview, so breaking change is expected

You mentioned that NettyNioAsyncHttpClient already supports configuration of both connectionTimeout and enableTcpKeepAlive and that the CRT client could share a common interface or base calss. Do you want to extract this interface as part of this CR (I assume to SdkAsyncHttpClient/SdkAsyncHttpClient.Builder, or leave it for a future change?

They should not share a common interface actually. It's by design because not every HTTP client supports the same client options.

nikp · 2022-10-04T18:41:41Z

I think they should be top-level fields. To be clear, we are going to just add connectionTimeout(Duration connectionTimeout) and tcpKeepAlive(Boolean tcpKeepAlive) right?

Maybe, I'm starting to second-guess it. Your reference to NettyNioAsyncHttpClient having a boolean field made me think that we already have configured defaults somewhere for keepAliveInterval and keepAliveTimeout but we do not - actually it looks like https://github.com/aws/aws-sdk-java-v2/blob/master/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/NettyNioAsyncHttpClient.java lets tcpKeepAlive be set on the Builder but isn't actually being forwarded to the client.

Which means that we either need to add sensible defaults for those values, or we let customers configure them. Since a sensible keep-alive interval very much depends on the underlying connection handler and their timeouts, I think it makes more sense to let customers configure them. Which creates weird validation edge-cases. Would it be better to just have the 2 Duration fields and if they're not set we treat it as tcpKeepAlive=false, and if they're set, true?

I think this should be fine. CRT HTTP Client is still in Developer Preview, so breaking change is expected

That's fine, but we should be explicit. We can't change the defaults for the underlying SocketOptions since that's in the CRT, and would affect other languages. But is DEFAULT_CONNECTION_TIMEOUT in the Java SDK used for other purposes or in other packages?

They should not share a common interface actually. It's be design because not every HTTP client supports the same client options.

An exercise for a future reader then.

zoewangg · 2022-10-04T19:06:07Z

Your reference to NettyNioAsyncHttpClient having a boolean field made me think that we already have configured defaults somewhere for keepAliveInterval and keepAliveTimeout but we do not

Yes, we do not have defaults for keepAliveInterval and keepAliveTimeout because the existing HTTP clients don't support them.

actually it looks like https://github.com/aws/aws-sdk-java-v2/blob/master/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/NettyNioAsyncHttpClient.java lets tcpKeepAlive be set on the Builder but isn't actually being forwarded to the client.

It it forwarded to the client through AttributeMap

aws-sdk-java-v2/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyConfiguration.java

Line 104 in e547f1f

return configuration.get(TCP_KEEPALIVE);

aws-sdk-java-v2/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/BootstrapProvider.java

Line 57 in e547f1f

.option(ChannelOption.SO_KEEPALIVE, nettyConfiguration.tcpKeepAlive())

But is DEFAULT_CONNECTION_TIMEOUT in the Java SDK used for other purposes or in other packages?

Yes, it's used by all HTTP clients that support connect timeout, i.e., Netty and Apache HTTP client.

Which means that we either need to add sensible defaults for those values, or we let customers configure them. Since a sensible keep-alive interval very much depends on the underlying connection handler and their timeouts, I think it makes more sense to let customers configure them. Which creates weird validation edge-cases. Would it be better to just have the 2 Duration fields and if they're not set we treat it as tcpKeepAlive=false, and if they're set, true?

I'd vote for adding sensible defaults and keeping tcpKeepAlive. Moved this conversion to #3457 (comment)

zoewangg · 2022-10-04T19:20:44Z

What would be considered the safe non-breaking way to resolve this inconsistency?

We could define an CRT HTTP defaults attribute map that has connect timeout of 3 seconds and merge the map in the build method. Example in netty:

aws-sdk-java-v2/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/NettyNioAsyncHttpClient.java

Lines 77 to 80 in e547f1f

    
           private static final AttributeMap NETTY_HTTP_DEFAULTS = 
        
               AttributeMap.builder() 
        
                           .put(SdkHttpConfigurationOption.CONNECTION_MAX_IDLE_TIMEOUT, Duration.ofSeconds(5)) 
        
                           .build();

aws-sdk-java-v2/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/NettyNioAsyncHttpClient.java

Lines 726 to 730 in e547f1f

    
           return new NettyNioAsyncHttpClient(this, standardOptions.build() 
        
                                                                   .merge(serviceDefaults) 
        
                                                                   .merge(NETTY_HTTP_DEFAULTS) 
        
                                                                   .merge(SdkHttpConfigurationOption.GLOBAL_HTTP_DEFAULTS));

nikp · 2022-10-05T03:36:16Z

Submitted new revision:

Standard SDK configuration now brought from SdkHttpConfigurationOption defaults
Consistent interface with Netty client for Duration connectionTimeout and Boolean tcpKeepAlive
Additional configuration objects to emphasize more advanced functionality:
- TcpKeepAliveConfiguration for callers that want to enable keep-alive in the client so need to specify the interval/timeout
- SocketOptionsConfiguration for callers that want to specify the advanced socket domain/type

...ents/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/AwsCrtAsyncHttpClient.java

nikp · 2022-10-05T18:33:20Z

OK I've removed SocketOptionsConfiguration to leave that for a future change, and applied the rest of the feedback.

...ents/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/AwsCrtAsyncHttpClient.java

zoewangg · 2022-10-05T21:50:55Z

LGTM. Running the builds now

zoewangg · 2022-10-05T22:41:56Z

@nikp looks like there are some checkstyle errors. We can also take it over here if that works better

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.1.2:check (checkstyle) on project aws-crt-client: Failed during checkstyle execution: 
There are 4 errors reported by Checkstyle 8.42 with software/amazon/awssdk/checkstyle.xml ruleset. -> [Help 1]

You can try locally using the following commands:

mvn clean install -pl :aws-crt-client -P quick --am
mvn clean install -pl :bom-internal
mvn clean install -pl :aws-crt-client

… and advanced SocketOptions on the AwsCrtAsyncHttpClient. This is necessary for any clients with long-running connections that exceed default socket timeouts of services along the call path, and need to enable keep-alive settings which the CRT client supports, but the Java client wasn't exposing to callers

zoewangg

Thank you for your contribution!

sonarqubecloud · 2022-10-06T20:42:07Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

65.9% Coverage
0.0% Duplication

nikp requested a review from a team as a code owner October 3, 2022 21:47

nikp force-pushed the master branch from 96b08f5 to 1f117d3 Compare October 3, 2022 21:48

JonathanHenson approved these changes Oct 3, 2022

View reviewed changes

nikp force-pushed the master branch from 1f117d3 to 5e2cbf3 Compare October 4, 2022 00:10

L-Applin reviewed Oct 4, 2022

View reviewed changes

...aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/SocketOptionsConfiguration.java Outdated Show resolved Hide resolved

L-Applin reviewed Oct 4, 2022

View reviewed changes

...aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/SocketOptionsConfiguration.java Outdated Show resolved Hide resolved

nikp mentioned this pull request Oct 4, 2022

Enable configuration of the connection timeout for AwsCrtAsyncHttpClient #3456

Closed

12 tasks

zoewangg reviewed Oct 4, 2022

View reviewed changes

nikp force-pushed the master branch 3 times, most recently from 3f0ec56 to 1f8981e Compare October 5, 2022 03:31

L-Applin reviewed Oct 5, 2022

View reviewed changes

...ents/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/AwsCrtAsyncHttpClient.java Outdated Show resolved Hide resolved

nikp force-pushed the master branch from 1f8981e to 2af287f Compare October 5, 2022 18:32

zoewangg reviewed Oct 5, 2022

View reviewed changes

...ents/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/AwsCrtAsyncHttpClient.java Show resolved Hide resolved

...ents/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/AwsCrtAsyncHttpClient.java Outdated Show resolved Hide resolved

nikp force-pushed the master branch 2 times, most recently from 5afb2ee to 252993f Compare October 5, 2022 21:08

nikp force-pushed the master branch 2 times, most recently from 7533fee to 9d25519 Compare October 6, 2022 04:55

nikp force-pushed the master branch from 9d25519 to 2c8d08f Compare October 6, 2022 16:36

Merge branch 'master' into master

6daf664

zoewangg approved these changes Oct 6, 2022

View reviewed changes

zoewangg merged commit 8cd6f5b into aws:master Oct 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to configure SocketOptions on the AwsCrtAsyncHttpClient #3457

Add the ability to configure SocketOptions on the AwsCrtAsyncHttpClient #3457

nikp commented Oct 3, 2022

JonathanHenson left a comment

L-Applin commented Oct 4, 2022

zoewangg Oct 4, 2022

nikp Oct 4, 2022

nikp Oct 4, 2022

zoewangg Oct 4, 2022

nikp Oct 4, 2022

JonathanHenson Oct 4, 2022

nikp Oct 4, 2022

zoewangg Oct 4, 2022

nikp Oct 5, 2022

zoewangg Oct 4, 2022

nikp Oct 4, 2022

JonathanHenson Oct 4, 2022

zoewangg Oct 4, 2022

nikp Oct 4, 2022

JonathanHenson Oct 4, 2022

zoewangg Oct 4, 2022

nikp Oct 4, 2022

nikp Oct 5, 2022

nikp commented Oct 4, 2022 •

edited

Loading

zoewangg commented Oct 4, 2022 •

edited

Loading

nikp commented Oct 4, 2022

zoewangg commented Oct 4, 2022

zoewangg commented Oct 4, 2022

nikp commented Oct 5, 2022

nikp commented Oct 5, 2022

zoewangg commented Oct 5, 2022

zoewangg commented Oct 5, 2022

zoewangg left a comment

sonarqubecloud bot commented Oct 6, 2022

	public Builder connectionTimeout(Duration timeout) {
	Validate.isPositive(timeout, "connectionTimeout");
	standardOptions.put(SdkHttpConfigurationOption.CONNECTION_TIMEOUT, timeout);
	return this;

	public static final AttributeMap GLOBAL_HTTP_DEFAULTS = AttributeMap
	.builder()
	.put(READ_TIMEOUT, DEFAULT_SOCKET_READ_TIMEOUT)
	.put(WRITE_TIMEOUT, DEFAULT_SOCKET_WRITE_TIMEOUT)
	.put(CONNECTION_TIMEOUT, DEFAULT_CONNECTION_TIMEOUT)
	.put(CONNECTION_ACQUIRE_TIMEOUT, DEFAULT_CONNECTION_ACQUIRE_TIMEOUT)
	.put(CONNECTION_MAX_IDLE_TIMEOUT, DEFAULT_CONNECTION_MAX_IDLE_TIMEOUT)
	.put(CONNECTION_TIME_TO_LIVE, DEFAULT_CONNECTION_TIME_TO_LIVE)
	.put(MAX_CONNECTIONS, DEFAULT_MAX_CONNECTIONS)
	.put(MAX_PENDING_CONNECTION_ACQUIRES, DEFAULT_MAX_CONNECTION_ACQUIRES)
	.put(PROTOCOL, DEFAULT_PROTOCOL)
	.put(TRUST_ALL_CERTIFICATES, DEFAULT_TRUST_ALL_CERTIFICATES)
	.put(REAP_IDLE_CONNECTIONS, DEFAULT_REAP_IDLE_CONNECTIONS)
	.put(TCP_KEEPALIVE, DEFAULT_TCP_KEEPALIVE)
	.put(TLS_KEY_MANAGERS_PROVIDER, DEFAULT_TLS_KEY_MANAGERS_PROVIDER)
	.put(TLS_TRUST_MANAGERS_PROVIDER, DEFAULT_TLS_TRUST_MANAGERS_PROVIDER)
	.put(TLS_NEGOTIATION_TIMEOUT, DEFAULT_TLS_NEGOTIATION_TIMEOUT)

Add the ability to configure SocketOptions on the AwsCrtAsyncHttpClient #3457

Add the ability to configure SocketOptions on the AwsCrtAsyncHttpClient #3457

Conversation

nikp commented Oct 3, 2022

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

JonathanHenson left a comment

Choose a reason for hiding this comment

L-Applin commented Oct 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikp commented Oct 4, 2022 • edited Loading

zoewangg commented Oct 4, 2022 • edited Loading

nikp commented Oct 4, 2022

zoewangg commented Oct 4, 2022

zoewangg commented Oct 4, 2022

nikp commented Oct 5, 2022

nikp commented Oct 5, 2022

zoewangg commented Oct 5, 2022

zoewangg commented Oct 5, 2022

zoewangg left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 6, 2022

nikp commented Oct 4, 2022 •

edited

Loading

zoewangg commented Oct 4, 2022 •

edited

Loading