[GitHub] [geode] upthewaterspout commented on issue #4963: GEODE-7503: Block Cache.close() until everything is disconnected

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

upthewaterspout commented on issue #4963:
URL: https://github.com/apache/geode/pull/4963#issuecomment-617442287


   The combination of a `ThreadLocal<Thread>`, a `CountDownLatch`, and `synchronized()` seems a bit complicated.
   
   The whole close block is already inside of  `synchronized(GemFireCacheImpl.class) {}`. It seems like the race condition is just with the early out at the beginning of close:
   
   ```
   if (isClosed()) {
         return;
   }
   ```
   
   Couldn't that just be changed to 
   
   ```
   if (!skipWait && isClosed()) {
         return;
   }
   ```
   
   To handle thread reentering cache.close, just don't add a skipWait check inside the synchronized block here:
   
   ```
   synchronized (GemFireCacheImpl.class) {
         // ALL CODE FOR CLOSE SHOULD NOW BE UNDER STATIC SYNCHRONIZATION OF GemFireCacheImpl.class
         // static synchronization is necessary due to static resources
         if (isClosed()) {
   
   ====> Remove the below check
           if (!skipAwait...)
   ```
   
   Any code that gets into the synchronized block is guaranteed that either (a) the cache has not been closed by another thread (b) the cache is completely closed already or (c) the thread is reentering cache close, in which case it should early out.
   
   There is probably some wrinkle that I'm missing here...


----------------------------------------------------------------
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.

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