IMO if it's not currently synchronous, that's just a bug that needs to be fixed. If folks can provide details on what actually was asynchronous or the particular test failures they saw, that would be helpful.
Previously, when this came up it looks like Kirk found that close would not wait for a different call to close() issued by a different thread [1]. Is that still the bug we are running into? One that thread, it seems like we also had a consensus we should just fix bugs with Cache.close: -Dan 1. https://lists.apache.org/x/thread.html/f385a6dd51209e2706c68c9821412a6f22ebef3e809636060ac0bf55@%3Cdev.geode.apache.org%3E On Tue, Apr 14, 2020 at 3:23 PM John Blum <jb...@pivotal.io> wrote: > My first thought is cache close (i.e. RegionService.close() should be > synchronous (by default), perhaps, with non-blocking options or options to > wait for a set timeout as Jake suggested. > > This is a problem for *Integration Tests* (that start a peer cache > instance, in-process or standalone) in general and not simply just > "distributed" tests! This is the reason I built support for this in > *Spring > Test for Apache Geode* (STDG) since subsequent tests requiring a peer cache > instance (or CacheServer) may conflict with each other, especially given 1) > the cache instance is a *Singleton* and 2) it is ideal to not have to > restart the JVM between, even for *Integration Tests*, however, turns out > to be a really common practice. *#ugh* However, without proper > coordination this can be a real problem, hence STDG's support. Even when > forking JVMs, you still have to be careful to wait in certain cases, as you > could run into other conflicts (e.g. BindExceptions if not varying port > numbers and such). For all these reasons and more, it is important that > the cache has fully shutdown and released all its resources. > > Also, while we are on this topic, I think it would be useful to have a > dedicated interface for the cache instance lifecycle. It's unfortunate the > CacheListener interface is already taken for Region events. *#sigh* > > The interface might be something like... > > interface CacheLifecycleListener { > > default void isStarting(CacheEvent event) {} > > default void onStart(CacheEvent event) {} > > default void isClosing(CacheEvent event) {} > > default void onClose(CacheEvent event) {} > > ... > > } > > -j > > On Tue, Apr 14, 2020 at 3:21 PM Jason Huynh <jhu...@pivotal.io> wrote: > > > The isClosed flag and method might be used currently more as an > isClosing. > > The GemFireCacheImpl.isClosed() method is actually returning isClosing. > > Whatever change to isClosed that will be made, will have to properly > handle > > cases where it's meant to be treated as isClosing(). > > > > On Tue, Apr 14, 2020 at 3:09 PM Mark Hanson <hans...@vmware.com> wrote: > > > > > Hi Jake, > > > > > > For Option 6: We could fix isClosed as well. That is a great > suggestion. > > > Currently, it returns almost immediately. > > > I like your options though.... > > > > > > Any other thoughts? > > > > > > Any preferences? It think any of the current options seem better than > the > > > current situation as long as we fix isClosed. > > > > > > Thanks, > > > Mark > > > ________________________________ > > > From: Jacob Barrett <jbarr...@pivotal.io> > > > Sent: Tuesday, April 14, 2020 2:30 PM > > > To: dev@geode.apache.org <dev@geode.apache.org> > > > Subject: Re: [Discuss] Cache.close synchronous is not synchronous, but > > > code still expects it to be.... > > > > > > Option 4: Cache.closeAndWait(long timeout, TimeUnit unit) - Closes and > > > waits until it is really closed. > > > Option 5: Cache.close(Runnable closedCalleback) - Runs callback after > > > cache is really close. > > > Option 6: cache.close(); while (!cache.isClosed()); > > > > > > > > > > On Apr 14, 2020, at 2:11 PM, Mark Hanson <mhan...@pivotal.io> wrote: > > > > > > > > Hi All, > > > > > > > > I know that we have discussed this once before, but I think it bears > > > repeating. We have test code that assumes cache.close is synchronous. > It > > is > > > not. Not even close. I would like discuss some possible changes. > > > > > > > > Option 1. Call it what it is. Deprecate Cache.close and create a new > > > method called asyncClose to replace it. Simple and descriptive. > > > > Option 2. Fix cache close so it is synchronous. Some might say that > we > > > are going to break behavior, but I doubt any user relies on the fact > that > > > it is asynchronous. That would be dangerous in and of itself. > Obviously, > > we > > > don’t want to change behavior, but there have been a number of > > distributed > > > tests that have failed for this. If internal to the code devs don’t get > > it > > > right, where does that leave users. > > > > Option 3. Status quo. > > > > > > > > What do you think? Are there other options I am missing? > > > > > > > > Thanks, > > > > Mark > > > > > > > > > > > > > > > -- > -John > Spring Data Team >
