I believe it is because of disk persistence among other things. Kirk would know for sure. He fixed the issue and his PR got shutdown. I totally support just fixing the bug.
Let’s give Kirk a chance to chime in. Thanks, Mark > On Apr 14, 2020, at 3:39 PM, Dan Smith <dsm...@pivotal.io> wrote: > > 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://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.apache.org%2Fx%2Fthread.html%2Ff385a6dd51209e2706c68c9821412a6f22ebef3e809636060ac0bf55%40%253Cdev.geode.apache.org%253E&data=02%7C01%7Chansonm%40vmware.com%7C7a43463ab53c416234d908d7e0c4cc6b%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637225008165230328&sdata=GD77kAubDDWfP93zjYsNP61VMN4%2FKBAHcq95GwjeMBc%3D&reserved=0 > > 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 >>
