PR comment changes

Author: Myron Scott

tc-client/src/main/java/com/tc/object/ClientEntityManagerImpl.java

@@ -79,6 +79,7 @@
 
 import static com.tc.object.EntityDescriptor.createDescriptorForLifecycle;
 import static com.tc.object.SafeInvocationCallback.safe;
+import java.util.concurrent.Callable;
 import static java.util.stream.Collectors.toCollection;
 import static org.terracotta.entity.Invocation.uninterruptiblyGet;
 
@@ -408,23 +409,20 @@ private <M extends EntityMessage, R extends EntityResponse> EntityClientEndpoint
     Assert.assertNotNull("Can't lookup null entity descriptor", instance);
     final EntityDescriptor fetchDescriptor = EntityDescriptor.createDescriptorForFetch(entity, version, instance);
     EntityClientEndpointImpl<M, R> resolvedEndpoint = null;
-    // make sure this close hook is only ever called once
-    CloseHookCallable closeCall = new CloseHookCallable() {
+    // make sure release is only ever called once
+    Callable<Void> closeCall = new Callable<Void>() {
       boolean released = false;
+      
       @Override
-      public EntityException call() {
+      public synchronized Void call() throws Exception {
         if (!released) {
           released = true;
-          try {
-            internalRelease(entity, fetchDescriptor);
-          } catch (EntityException e) {
-            // We aren't expecting there to be any problems releasing an entity in the close hook but return the exception if there is an issue.
-            return e;
-          }
+          internalRelease(entity, fetchDescriptor);
         }
         return null;
       }
     };
+    
     try {
       byte[] raw = internalRetrieve(fetchDescriptor);
       ByteBuffer br = ByteBuffer.wrap(raw);
@@ -445,13 +443,21 @@ public EntityException call() {
       throw notfound;
     } catch (EntityException e) {
       // Release the entity and re-throw to the higher level.
-      e.addSuppressed(closeCall.call());
+      try {
+        closeCall.call();
+      } catch (Exception subE) {
+        e.addSuppressed(subE);
+      }
 
       throw e;
     } catch (Throwable t) {
       // This is the unexpected case so clean up and re-throw as a RuntimeException
       // Clean up any client-side or server-side state regarding this failed connection.
-      t.addSuppressed(closeCall.call());
+      try {
+        closeCall.call();
+      } catch (Exception subE) {
+        t.addSuppressed(subE);
+      }
 
       throw Throwables.propagate(t);
     }

tc-client/src/main/java/com/tc/object/CloseHookCallable.java

@@ -53,7 +53,7 @@ public class EntityClientEndpointImpl<M extends EntityMessage, R extends EntityR
   private final EntityID entityID;
   private final long version;
   private final MessageCodec<M, R> codec;
-  private final CloseHookCallable closeHook;
+  private final Callable<Void> closeHook;
   private final ExecutorService closer;
   private EndpointDelegate<R> delegate;
   private boolean isOpen;
@@ -67,7 +67,7 @@ public class EntityClientEndpointImpl<M extends EntityMessage, R extends EntityR
    * @param entityConfiguration Opaque byte[] describing how to configure the entity to be built on top of this end-point.
    * @param closeHook A Runnable which will be run last when the end-point is closed.
    */
-  public EntityClientEndpointImpl(EntityID eid, long version, EntityDescriptor instance, InvocationHandler invocationHandler, byte[] entityConfiguration, MessageCodec<M, R> codec, CloseHookCallable closeHook, ExecutorService closer) {
+  public EntityClientEndpointImpl(EntityID eid, long version, EntityDescriptor instance, InvocationHandler invocationHandler, byte[] entityConfiguration, MessageCodec<M, R> codec, Callable<Void> closeHook, ExecutorService closer) {
     this.entityID = eid;
     this.version = version;
     this.invokeDescriptor = instance;
@@ -97,14 +97,14 @@ EntityDescriptor getEntityDescriptor() {
   @Override
   public byte[] getEntityConfiguration() {
     // This is harmless while closed but shouldn't be called so check open.
-    checkEndpointOpen(false);
+    checkEndpointOpen();
     return configuration;
   }
 
   @Override
   public void setDelegate(EndpointDelegate<R> delegate) {
     // This is harmless while closed but shouldn't be called so check open.
-    checkEndpointOpen(false);
+    checkEndpointOpen();
     Assert.assertNull(this.delegate);
     this.delegate = delegate;
   }
@@ -125,7 +125,7 @@ public InFlightStats getStatistics() {
   @Override
   public Invocation<R> message(M message) {
     // We can't create new invocations when the endpoint is closed.
-    checkEndpointOpen(false);
+    checkEndpointOpen();
     return new InvocationImpl(message);
   }
 
@@ -177,16 +177,15 @@ public byte[] getExtendedReconnectData() {
   @Override
   public void close() {
     // We can't close twice.
-    try {
-      checkEndpointOpen(true);
-    } catch (IllegalStateException alreadyClosed) {
-      LOGGER.info("Trying to close an endpoint which has already been closed", alreadyClosed);
-      return;
-    }
-    if (this.closeHook != null) {
-      EntityException e = this.closeHook.call();
-      LOGGER.info("Exception occured during close", e);
-      // log and swallow this exception closing
+    if (closeIfOpen()) {
+      if (this.closeHook != null) {
+        try {
+          this.closeHook.call();
+        } catch (Exception e) {
+          LOGGER.warn("Exception occured during close", e);
+        }
+        // log and swallow this exception closing
+      }
     }
   }
 
@@ -226,12 +225,15 @@ public void didCloseUnexpectedly() {
     }
   }
 
-  private synchronized void checkEndpointOpen(boolean close) {
+  private synchronized void checkEndpointOpen() {
     if (!this.isOpen) {
       throw new IllegalStateException("Endpoint closed");
     }
-    if (close) {
-      this.isOpen = false;
-    }
+  }
+
+  private synchronized boolean closeIfOpen() {
+    boolean wasOpen = this.isOpen;
+    this.isOpen = false;
+    return wasOpen;
   }
 }

tc-client/src/main/java/com/tc/object/EntityClientEndpointImpl.java

@@ -987,14 +987,13 @@ private void getEntity(ServerEntityRequest getEntityRequest, ResultCapture respo
           Assert.assertTrue(clientReferenceCount > 0);
         } else {
           // this should never happen.  Ther server is expecting sane things from the client. 
-          logger.info("the client has attempted to fetch the same entity instance twice {}", descriptor);
+          logger.warn("the client has attempted to fetch the same entity instance twice {}", descriptor);
         }
       }
 
       if (this.isInActiveState) {
         if (!added) {
-          //  this should never happen if the client is sane but if it does, exception the client
-          //  don't crash the server
+          //  Exception the client.  Don't crash the server
           response.failure(ServerException.createReferencedException(id));
           return;
         }
@@ -1060,15 +1059,14 @@ private void releaseEntity(ServerEntityRequest request, ResultCapture response)
           clientReferenceCount -= 1;
           Assert.assertTrue(clientReferenceCount >= 0);
         } else {
-        // this should never happen.  the expectation is that the client will send sane things to the server.  Will assert if active
-          logger.info("client has tried to release and entity that is not fetched {}", clientInstance);
+        // this should never happen.  the expectation is that the client will send sane things to the server.
+          logger.warn("client has tried to release an entity that is not fetched {}", clientInstance);
         }
       }
 
       if (this.isInActiveState) {
         if (!removed) {
-          //  this should never happen if the client is sane but if it does, exception the client
-          //  don't crash the server
+          //  Exception the client.  don't crash the server
           response.failure(ServerException.createNotFoundException(id));
           return;
         }