GEODE-6164: CacheClientProxy's closeSocket should be called atomically by gesterzhou · Pull Request #2972 · apache/geode

gesterzhou · 2018-12-07T07:28:47Z

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically develop)?
Is your initial contribution a single, squashed commit?
Does gradlew build run cleanly?
Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

jhuynh1 · 2018-12-07T19:36:25Z

geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheClientProxy.java

+    if (!closeSocket()) {
+      // other thread who closed the socket will be responsible to
+      // releaseResourcesForAddress and clearClientInterestList
+      return;


The callers of this method will possibly return without doing the rest of the work. As long as the callers are ok/expecting this to occur, then this should be ok...

jhuynh1 · 2018-12-07T22:06:48Z

.../integrationTest/java/org/apache/geode/internal/cache/tier/sockets/CacheClientProxyTest.java

+
+    CacheServerStats stats = mock(CacheServerStats.class);
+    doNothing().when(stats).incCurrentQueueConnections();
+    // InternalCache cache = cacheRule.getCache();


Remove this comment?

jhuynh1 · 2018-12-07T22:09:36Z

.../integrationTest/java/org/apache/geode/internal/cache/tier/sockets/CacheClientProxyTest.java

+    ((CompletableFuture<Void>) result1).join();
+    ((CompletableFuture<Void>) result2).join();
+    ((CompletableFuture<Void>) result3).join();
+    assertThatCode(() -> result1.get(2, SECONDS)).doesNotThrowAnyException();


Should this be bumped up to a higher number? Maybe just have it have a max wait of 60 seconds, that way it won't fail if for some reason the close happens to take longer or maybe the vm we are running on got cpu starved for a bit?

jhuynh1 · 2018-12-07T22:10:55Z

geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheClientProxy.java

-    if (this._socketClosed.compareAndSet(false, true)) {
+  private boolean closeSocket() {
+    String remoteHostAddress = this._remoteHostAddress;
+    if (this._socketClosed.compareAndSet(false, true) && remoteHostAddress != null) {


Would it make sense to break the && condition into another if call instead?

if (remoteHostAddress == null) {
return false?
}

I think it's worth comments above each of closeSocket() and closeTransientFields() explaining that _socketClosed is used to provide one-shot protection around the close logic in both methods.

jhuynh1 · 2018-12-07T22:11:43Z

geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheClientProxy.java


  private void closeTransientFields() {
-    closeSocket();
+    if (!closeSocket()) {


What happens if remoteHostAddress is null, we end up returning false from closeSocket and not doing the rest of this method. Is that acceptable?

remoteHostAddress is null only when closeTransientFields() is called. So it's ok not to do anything else.

Bill

Good stuff @gesterzhou. I made a couple suggestions that might add a little bit to readability/maintainability. Functionally this looks good to me.

Bill · 2018-12-07T23:34:14Z

.../integrationTest/java/org/apache/geode/internal/cache/tier/sockets/CacheClientProxyTest.java

+    CacheClientProxy proxy = new CacheClientProxy(ccn, socket, proxyID, true,
+        Handshake.CONFLATION_DEFAULT, Version.CURRENT, 1L, true,
+        null, null);
+    Future<Void> result1 = executorServiceRule.runAsync(() -> proxy.close());


runAsync() returns a CompletableFuture<>. Since you'll need a CompletableFuture later (e.g. line 77), why not capture a CompletableFuture<Void> ref here to avoid the cast later?

Bill · 2018-12-07T23:37:12Z

.../integrationTest/java/org/apache/geode/internal/cache/tier/sockets/CacheClientProxyTest.java

+    ((CompletableFuture<Void>) result3).join();
+    assertThatCode(() -> result1.get(60, SECONDS)).doesNotThrowAnyException();
+    assertThatCode(() -> result2.get(60, SECONDS)).doesNotThrowAnyException();
+    assertThatCode(() -> result3.get(60, SECONDS)).doesNotThrowAnyException();


FYI if you capture CompletableFuture<> refs above, then these three joins can become one statement:

CompletableFuture.allOf(result1,result2,result3).join();

sounds good. I enhanced the code.

Bill · 2018-12-07T23:41:58Z

.../integrationTest/java/org/apache/geode/internal/cache/tier/sockets/CacheClientProxyTest.java

+    InetAddress address = mock(InetAddress.class);
+    when(socket.getInetAddress()).thenReturn(address);
+    when(address.getHostAddress()).thenReturn("localhost");
+    doNothing().when(sc).asyncClose(any(), eq("localhost"), eq(null));


This would read a lot easier for me if:

variables that could be declared final (all of them!) were declared final

vertical space was used to separate each mocked object (its construction and configuration via when(...))

Bill · 2018-12-07T23:48:43Z

geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheClientProxy.java

-    if (this._socketClosed.compareAndSet(false, true)) {
+  private boolean closeSocket() {
+    String remoteHostAddress = this._remoteHostAddress;
+    if (this._socketClosed.compareAndSet(false, true) && remoteHostAddress != null) {


I think it's worth comments above each of closeSocket() and closeTransientFields() explaining that _socketClosed is used to provide one-shot protection around the close logic in both methods.

Bill · 2018-12-08T00:16:50Z

Looks good to me now.

gesterzhou force-pushed the feature/GEODE-6164 branch 2 times, most recently from 85dde7d to 4751056 Compare December 7, 2018 22:04

jhuynh1 reviewed Dec 7, 2018

View reviewed changes

gesterzhou force-pushed the feature/GEODE-6164 branch from 4751056 to e78b493 Compare December 7, 2018 22:33

Bill reviewed Dec 7, 2018

View reviewed changes

gesterzhou force-pushed the feature/GEODE-6164 branch from e78b493 to 2fed76c Compare December 7, 2018 23:58

GEODE-6164: CacheClientProxy's closeSocket should be called atomically

77b3926

gesterzhou force-pushed the feature/GEODE-6164 branch from 2fed76c to 77b3926 Compare December 8, 2018 00:02

gesterzhou merged commit 4fde138 into develop Dec 8, 2018

gesterzhou deleted the feature/GEODE-6164 branch December 8, 2018 01:50

Conversation

gesterzhou commented Dec 7, 2018

For all changes:

Note:

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!

Bill left a comment

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!

Bill commented Dec 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants