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:
us...@infra.apache.org
