Fix ApacheHttpClient to use socket keep alive when appropriate #2373

ajs139 · 2021-04-05T19:09:17Z

Description

This enables the underlying Socket.SO_KEEPALIVE when the CONNECTION_KEEPALIVE configuration option is set to true.

Motivation and Context

There's a known issue when using a NAT gateway that it times out after 350 seconds if the connection is not kept alive. If using ApacheHttpClient to create connections to an AWS Lambda from a machine within the VPC it appears that this connection goes through the NAT. This means that any Lambda invocations taking more than 350 seconds will never get a response.

Testing

Deployed the code to environment that was experiencing the timeout issue, and verified that setting setSoKeepAlive(true) on the SocketConfig enabled keep-alive and allowed connections running longer than 350 seconds to respond.

Screenshots (if appropriate)

Types of changes

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

Checklist

License

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

zoewangg · 2021-04-05T22:50:32Z

Thank you for the PR! We will take a look shortly

zoewangg

Could you add a changelog entry? You can generate one by running the scripts/new-change script and following the instructions.

zoewangg · 2021-04-06T21:42:31Z

...clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/ApacheHttpClient.java

+        /**
+         * Configure the connection to use java.net.SocketOptions.SO_KEEPALIVE.
+         */
+        Builder connectionKeepalive(Boolean keepConnectionAlive);


Same as above. Can we change it to tcpKeepAlive?

zoewangg · 2021-04-06T21:43:51Z

...clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/ApacheHttpClient.java

@@ -675,10 +693,10 @@ public void checkServerTrusted(X509Certificate[] x509Certificates, String s) thr
            };
        }

-        private SocketConfig buildSocketConfig(AttributeMap standardOptions) {
+        private SocketConfig buildSocketConfig(AttributeMap standardOptions,
+                                                  Boolean soKeepAlive) {


Can we get soKeepAlive from standardOptions.get(SdkHttpConfigurationOption.TCP_KEEPALIVE) directly instead of passing it?

zoewangg · 2021-04-06T21:52:18Z

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

+    /**
+     * Whether or not to use keepalive on the connection.
+     */
+    public static final SdkHttpConfigurationOption<Boolean> CONNECTION_KEEPALIVE =


Can we rename it to TCP_KEEPALIVE so that users don't confuse it with HTTP keep alive?

zoewangg · 2021-04-06T21:53:25Z

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

@@ -116,6 +122,7 @@
    private static final Duration DEFAULT_CONNECTION_TIMEOUT = Duration.ofSeconds(2);
    private static final Duration DEFAULT_CONNECTION_ACQUIRE_TIMEOUT = Duration.ofSeconds(10);
    private static final Duration DEFAULT_CONNECTION_MAX_IDLE_TIMEOUT = Duration.ofSeconds(60);
+    private static final Boolean DEFAULT_CONNECTION_KEEPALIVE = Boolean.TRUE;


We should probably make it default to false so that it has the same behavior as now.

Sorry, thought it was, not sure what happened there.

ajs139 · 2021-04-07T01:40:46Z

@zoewangg - thanks for the code review, it's much cleaner now.

codecov-io · 2021-04-07T02:16:12Z

Codecov Report

Merging #2373 (fb10a32) into master (e5b53e1) will decrease coverage by 0.02%.
The diff coverage is 12.50%.

❗ Current head fb10a32 differs from pull request most recent head 8aa2889. Consider uploading reports for the commit 8aa2889 to get more accurate results

@@             Coverage Diff              @@
##             master    #2373      +/-   ##
============================================
- Coverage     77.68%   77.66%   -0.03%     
  Complexity      366      366              
============================================
  Files          1246     1245       -1     
  Lines         39358    39301      -57     
  Branches       3110     3106       -4     
============================================
- Hits          30577    30522      -55     
  Misses         7291     7291              
+ Partials       1490     1488       -2

Flag	Coverage Δ	Complexity Δ
unittests	`77.66% <12.50%> (-0.03%)`	`0.00 <0.00> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...amazon/awssdk/http/SdkHttpConfigurationOption.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...re/amazon/awssdk/http/apache/ApacheHttpClient.java	`75.58% <20.00%> (-1.45%)`	`0.00 <0.00> (ø)`
...k/core/pagination/async/ResponsesSubscription.java	`72.00% <0.00%> (-16.00%)`	`0.00% <0.00%> (ø%)`
...nio/netty/internal/OldConnectionReaperHandler.java	`81.81% <0.00%> (-9.10%)`	`0.00% <0.00%> (ø%)`
...mazon/awssdk/utils/internal/MappingSubscriber.java	`82.75% <0.00%> (-6.90%)`	`0.00% <0.00%> (ø%)`
...ore/client/config/ClientOverrideConfiguration.java	`77.57% <0.00%> (-1.92%)`	`0.00% <0.00%> (ø%)`
.../awscore/client/handler/AwsClientHandlerUtils.java	`90.16% <0.00%> (-0.89%)`	`0.00% <0.00%> (ø%)`
...k/core/client/builder/SdkDefaultClientBuilder.java	`90.97% <0.00%> (-0.07%)`	`0.00% <0.00%> (ø%)`
...zon/awssdk/core/client/config/SdkClientOption.java	`100.00% <0.00%> (ø)`	`0.00% <0.00%> (ø%)`
.../core/internal/handler/BaseAsyncClientHandler.java	`94.31% <0.00%> (ø)`	`0.00% <0.00%> (ø%)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5b53e1...8aa2889. Read the comment docs.

zoewangg

Thanks for updating the PR! Just have one minor comment, otherwise lgtm.

...clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/ApacheHttpClient.java

zoewangg · 2021-04-07T21:19:46Z

...clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/ApacheHttpClient.java

+            return this;
+        }
+
+        public void setTcpKeepalive(Boolean keepConnectionAlive) {


It seems the setter also needs to be updated to setTcpKeepAlive

Missed that one, thank you.

zoewangg

LGTM. Thank you for your contribution!

sonarqubecloud · 2021-04-07T22:13:18Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

12.5% Coverage
4.2% Duplication

zoewangg · 2021-04-08T00:20:02Z

@all-contributors please add @ajs139 for code

allcontributors · 2021-04-08T00:20:11Z

@zoewangg

I've put up a pull request to add @ajs139! 🎉

…bfe69efaa Pull request: release <- staging/1611ed87-a79f-4f4f-88a8-66ebfe69efaa

ajs139 force-pushed the fix/keep-alive branch 2 times, most recently from f8f2e17 to d01d32e Compare April 5, 2021 20:22

ajs139 force-pushed the fix/keep-alive branch from d01d32e to 254977f Compare April 6, 2021 12:56

zoewangg reviewed Apr 6, 2021

View reviewed changes

ajs139 force-pushed the fix/keep-alive branch 3 times, most recently from a1ffc01 to 8aa2889 Compare April 7, 2021 01:38

zoewangg reviewed Apr 7, 2021

View reviewed changes

...clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/ApacheHttpClient.java Outdated Show resolved Hide resolved

ajs139 force-pushed the fix/keep-alive branch from 8aa2889 to f13b94c Compare April 7, 2021 19:17

zoewangg reviewed Apr 7, 2021

View reviewed changes

Fix to ApacheHttpClient to allow socket keepalive

f47bed6

ajs139 force-pushed the fix/keep-alive branch from f13b94c to f47bed6 Compare April 7, 2021 21:24

zoewangg approved these changes Apr 7, 2021

View reviewed changes

Merge branch 'master' into fix/keep-alive

1316113

zoewangg merged commit 71dfc2c into aws:master Apr 8, 2021

allcontributors bot mentioned this pull request Apr 8, 2021

docs: add ajs139 as a contributor #2391

Merged

ajs139 deleted the fix/keep-alive branch April 8, 2021 00:21

aws-sdk-java-automation added a commit that referenced this pull request Feb 9, 2023

Merge pull request #2373 from aws/staging/1611ed87-a79f-4f4f-88a8-66e…

5f9574b

…bfe69efaa Pull request: release <- staging/1611ed87-a79f-4f4f-88a8-66ebfe69efaa

Fix ApacheHttpClient to use socket keep alive when appropriate #2373

Fix ApacheHttpClient to use socket keep alive when appropriate #2373

Uh oh!

Conversation

ajs139 commented Apr 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

zoewangg commented Apr 5, 2021

Uh oh!

zoewangg left a comment

Choose a reason for hiding this comment

Uh oh!

zoewangg Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

zoewangg Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

zoewangg Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

zoewangg Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

ajs139 Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

ajs139 commented Apr 7, 2021

Uh oh!

codecov-io commented Apr 7, 2021

Codecov Report

Uh oh!

zoewangg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zoewangg Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

ajs139 Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

zoewangg left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Apr 7, 2021

Uh oh!

zoewangg commented Apr 8, 2021

Uh oh!

allcontributors bot commented Apr 8, 2021

Uh oh!

Uh oh!

ajs139 commented Apr 5, 2021 •

edited

Loading