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
