Updated connection lifecycle behavior. #1055

millems · 2019-02-01T21:48:52Z

Added support for useIdleConnectionReaper, connectionTimeToLive and connectionMaxIdleTime to the Netty HTTP client.
Enable the idle connection reaper by default for apache and netty clients. Connection time-to-live remains disabled by default.

Fixes #856

codecov-io · 2019-02-01T22:15:14Z

Codecov Report

Merging #1055 into master will increase coverage by 0.05%.
The diff coverage is 75.2%.

@@             Coverage Diff              @@
##             master    #1055      +/-   ##
============================================
+ Coverage     55.96%   56.02%   +0.05%     
- Complexity     4614     4648      +34     
============================================
  Files           815      818       +3     
  Lines         27824    27915      +91     
  Branches       2247     2253       +6     
============================================
+ Hits          15572    15639      +67     
- Misses        11532    11549      +17     
- Partials        720      727       +7

Impacted Files	Coverage Δ	Complexity Δ
...sdk/http/apache/internal/DefaultConfiguration.java	`100% <ø> (ø)`	`1 <0> (ø)`	⬇️
...amazon/awssdk/http/SdkHttpConfigurationOption.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...nio/netty/internal/ChannelPipelineInitializer.java	`44.44% <100%> (+5.66%)`	`6 <1> (+2)`	⬆️
...dk/http/nio/netty/internal/NettyConfiguration.java	`92.85% <100%> (+1.94%)`	`11 <3> (+3)`	⬆️
...k/http/nio/netty/internal/ChannelAttributeKey.java	`91.66% <100%> (+0.75%)`	`2 <0> (ø)`	⬇️
...re/amazon/awssdk/http/apache/ApacheHttpClient.java	`61.49% <50%> (-0.94%)`	`17 <1> (-1)`
...o/netty/internal/http2/HttpOrHttp2ChannelPool.java	`65.67% <62.5%> (-1%)`	`15 <2> (ø)`
...io/netty/internal/IdleConnectionReaperHandler.java	`72.72% <72.72%> (ø)`	`4 <4> (?)`
...awssdk/http/nio/netty/NettyNioAsyncHttpClient.java	`66.92% <73.91%> (+0.55%)`	`18 <1> (+1)`	⬆️
...nio/netty/internal/OldConnectionReaperHandler.java	`90.9% <90.9%> (ø)`	`15 <15> (?)`
... and 5 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 16b0c9c...32562c8. Read the comment docs.

dagnir · 2019-02-01T23:08:24Z

...src/main/java/software/amazon/awssdk/http/nio/netty/internal/OldConnectionReaperHandler.java

+    }
+
+    private void closeChannel(ChannelHandlerContext ctx) {
+        assert ctx.executor().inEventLoop();


Think we can avoid this by scheduling this within the event loop instead of the executor

ctx.channel().eventLoop()

zoewangg · 2019-02-01T23:47:52Z

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

+    /**
+     * Whether idle connection should be removed after the {@link #CONNECTION_MAX_IDLE_TIMEOUT} has passed.
+     */
+    public static final SdkHttpConfigurationOption<Boolean> REAP_IDLE_CONNECTIONS =


minor, how about REAP_IDLE_CONNECTIONS_ENABLED or ENABLE_REAP_IDLE_CONNECTIONS?

I was following the naming convention of TRUST_ALL_CERTIFICATES, which we can't change now. I'd rather be consistent.

zoewangg · 2019-02-01T23:52:42Z

...rc/main/java/software/amazon/awssdk/http/nio/netty/internal/IdleConnectionReaperHandler.java

+ * A handler that closes unused channels that have not had any traffic on them for a configurable amount of time.
+ */
+@SdkInternalApi
+public class IdleConnectionReaperHandler extends IdleStateHandler {


Can we add some unit tests for this class?

Its behavior is tested at build time with the functional tests.

In my opinion, that is better and sufficient for this class. Direct unit tests would be too coupled to the actual class implementation, and would make it impossible to evolve the class over time without having to go back and update the unit tests.

zoewangg · 2019-02-02T00:01:09Z

...main/java/software/amazon/awssdk/http/nio/netty/internal/HonorCloseOnReleaseChannelPool.java

+                channel.close();
+            }
+
+            delegatePool.release(channel, promise);


If the channel is closed on line 65, do we still do delegatePool.release(channel, promise)?

ChannelPool#acquire() documents: <strong>Its important that an acquired is always released to the pool again, even if the {@link Channel} is explicitly closed..</strong>

zoewangg · 2019-02-02T00:03:02Z

...main/java/software/amazon/awssdk/http/nio/netty/internal/HonorCloseOnReleaseChannelPool.java

+ * before it is released to the underlying pool.
+ */
+@SdkInternalApi
+public class HonorCloseOnReleaseChannelPool implements ChannelPool {


Can we add some unit tests for this class? Let's add a test class for every new class :)

I disagree. Too many class-level tests for simple classes with a large number of dependencies (like this one) makes the code base too rigid and hard to update. Class-level tests written for this class: (a) wouldn't verify it actually works, just that it calls the methods we think we should, and (b) would almost certainly have to be updated if we ever touch the implementation.

This class's behavior is better tested (imo) with the functional tests, run at build time.

Discussed in person. We agree that unit test class for IdleConnectionReaperHandler.java is not needed since the functional tests would cover it. However, the functional test might not cover this class, so unit tests for this class are needed. (Fun fight! 🍻)

Added tests for the release-on-close behavior.

zoewangg · 2019-02-04T19:59:13Z

...test/java/software/amazon/awssdk/http/nio/netty/internal/OldConnectionReaperHandlerTest.java

@@ -0,0 +1,49 @@
+package software.amazon.awssdk.http.nio.netty.internal;


Hmm, seems missing copyright headers on the test classes. (We need to add the checkstyles for test files)

1. Added support for `useIdleConnectionReaper`, `connectionTimeToLive` and `connectionMaxIdleTime` to the Netty HTTP client. 2. Enable the idle connection reaper by default for apache and netty clients. Connection time-to-live remains disabled by default. Fixes #856

…a16bed24d Pull request: release <- staging/c640cb78-709b-4cf8-95b2-b8ea16bed24d

millems force-pushed the millem/netty-idle-connection-reaper branch from 74a2f3b to 8d123ee Compare February 1, 2019 21:49

dagnir reviewed Feb 1, 2019

View reviewed changes

zoewangg reviewed Feb 2, 2019

View reviewed changes

millems force-pushed the millem/netty-idle-connection-reaper branch 3 times, most recently from 5fbdc54 to 7135f60 Compare February 4, 2019 19:44

zoewangg approved these changes Feb 4, 2019

View reviewed changes

millems force-pushed the millem/netty-idle-connection-reaper branch from 7135f60 to 32337e4 Compare February 4, 2019 20:01

Merge branch 'master' into millem/netty-idle-connection-reaper

32562c8

millems merged commit 71ca4c1 into master Feb 4, 2019

millems deleted the millem/netty-idle-connection-reaper branch March 17, 2020 23:39

aws-sdk-java-automation added a commit that referenced this pull request Nov 24, 2020

Merge pull request #1055 from aws/staging/c640cb78-709b-4cf8-95b2-b8e…

8fdc78f

…a16bed24d Pull request: release <- staging/c640cb78-709b-4cf8-95b2-b8ea16bed24d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Updated connection lifecycle behavior. #1055

Updated connection lifecycle behavior. #1055

Uh oh!

millems commented Feb 1, 2019

Uh oh!

codecov-io commented Feb 1, 2019 •

edited

Loading

Uh oh!

dagnir Feb 1, 2019

Uh oh!

millems Feb 2, 2019

Uh oh!

zoewangg Feb 1, 2019

Uh oh!

millems Feb 2, 2019

Uh oh!

zoewangg Feb 1, 2019

Uh oh!

millems Feb 2, 2019

Uh oh!

zoewangg Feb 2, 2019

Uh oh!

millems Feb 2, 2019

Uh oh!

zoewangg Feb 2, 2019

Uh oh!

millems Feb 2, 2019 •

edited

Loading

Uh oh!

zoewangg Feb 2, 2019

Uh oh!

millems Feb 4, 2019

Uh oh!

zoewangg Feb 4, 2019

Uh oh!

millems Feb 4, 2019

Uh oh!

Uh oh!

		@@ -0,0 +1,49 @@
		package software.amazon.awssdk.http.nio.netty.internal;

Updated connection lifecycle behavior. #1055

Updated connection lifecycle behavior. #1055

Uh oh!

Conversation

millems commented Feb 1, 2019

Uh oh!

codecov-io commented Feb 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

millems Feb 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Feb 1, 2019 •

edited

Loading

millems Feb 2, 2019 •

edited

Loading