[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7357: GEODE-10042: do not make ClientUserAuths null when we are not unregister client yet.

[GitBox <gi...@apache.org>]

dschneider-pivotal commented on a change in pull request #7357:
URL: https://github.com/apache/geode/pull/7357#discussion_r804920239



##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/CacheClientProxy.java
##########
@@ -812,6 +795,28 @@ else if (clientUserAuths != null) {
     return keepProxy;
   }
 
+  // this needs to synchronized to avoid NPE between null check and operations
+  private synchronized void cleanClientAuths() {

Review comment:
       It seems wrong to synchronize on "this" for this method when everything that sets the fields that it is using and nulling (postAuthzCallback, subject, clientUserAuths) are set while synchronized on "clientUserAuthsLock". So shouldn't this method also synchronize on clientUserAuthsLock?

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java
##########
@@ -959,9 +960,12 @@ public boolean isTerminated() {
     }
   }
 
-  private void cleanClientAuths() {
+  // this needs to be synchronized to avoid NPE betweeen null check and cleanup
+  @VisibleForTesting
+  synchronized void cleanClientAuths() {

Review comment:
       this synchronize does not protect threads calling cleanClientAuths from threads concurrently setting this field since they are not synchronized on "this". I'm also concerned that making this method synchronize on "this" on such a large class could have unexpected complications with other code on this class that synchronizes on "this". I think a better pattern to protect this code from having an NPE due to two threads calling it concurrently is to only read the field "clientUserAuths" once into a local var. Then use the local var to check if it is null and to call cleanup. This does mean that two threads could both call cleanup on the same instance of clientUserAuths.
   I think the cleanest way to prevent this is add a `final Object clientUserAuthsLock = new Object();` instance field and any code that want to set clientUserAuthsLock would synchronize on it (instead of this).
    This same class had issues with NPE when it trying to call a method on clientUserAuths and it fixed these by catching and eating the NPE. It seems like this code could have also read into a temp var and then checked if it was non-null and then called the method on the temp var so that the NPE never happens. If you want to make sure you are not calling methods on clientUserAuths instance that has had cleanup called on it then you could have these places that use clientUserAuths also be protected by the clientUserAuths lock. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org