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

["upthewaterspout (GitHub)" <gi...@apache.org>]

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

[ Full content available at: https://github.com/apache/geode/pull/4963 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org