• Bug
  • Status: Closed
  • 2 Major
  • Resolution: Won't Fix
  • asingh
  • Reporter: asingh
  • February 17, 2010
  • 0
  • Watchers: 3
  • May 21, 2010
  • May 07, 2010

Attachments

Description

There’s a CacheShutdownHook in tim-ehcache-2.0 that registers caches to be dispose’d off before L1 shutdown. Right now, it calls cache.setNodeCoherent(true) before L1 shutdown (so that in case its in incoherent mode, uncommitted buffered changes are committed to server).

Ideally, cache.dispose() should be called from CacheShutdownHook. But seems like multiple shutdown hooks can call cache.dispose() (the normal L1 shutdown hook and others like jetty, see stacktrace at DEV-3814). And ehcache right now throws exception calling dispose() multiple times.

Probably should fix ehcache Cache.dispose() to ignore multiple calls and use cache.dispose() in CacheShutdownHook. Not sure how changing Cache.dispose() to ignore multiple calls might affect. Other public users of ehcache may be already relying on cache.dispose() to throw exception on subsequent calls (looks weird, though possible)

Comments

Tim Eck 2010-02-17

I don’t think there were multiple dispose() in DEV-3814. The prob was that dispose() was getting called by a shutdown hook, but after the shutdown hook that stopped the L1.

Fiona OShea 2010-02-23

what needs to be done and when?

Tim Eck 2010-02-23

I don’t know that I can speak authoritatively on this issue and the last question. Abhishek care to comment?

Saravanan Subbiah 2010-02-23

Basically we are calling cache.setCoherent(true) when the node is shutdown expecting that this call will not return until all updates are flushed and the cache become coherent. But this is depending on todays implementation which can change. We were discussing in another meeting that we could make this call asynchronous, in which case this assumption will be broken.

The ideal thing seems like we should call cache.dispose() in the shutdown hook and that does the right thing. The problem is in containers like jetty, the container also calls cache.dispose() and calling dispose twice today throws an exception in Ehcache. If we fix that, then we can change the shutdown hook to call dispose instead of calling setCoherent()

Abhishek Singh 2010-04-14

Greg, do you have any problems if we change Cache.dispose() to not throw IllegalStateException anymore on calling dispose() multiple times? Basically code change is just removing call to checkStatusNotDisposed() in cache.dispose() and return if its already in STATUS_SHUTDOWN

Abhishek Singh 2010-04-14

Attached patch.

Abhishek Singh 2010-05-07

Won’t be fixing this for time being. Mail extract: I have committed the changes in ehcache-core whereby calling cache.dispose() multiple times will not throw IllegalArgumentException anymore. I am now thinking if we should change the call to cache.dispose() in the shutdown hook (from cache.setNodeCoherent(true)).

Doing so is a problem right now. We register caches for shutdown hook when initializing caches. And we unregister the caches from the shutdown hook when the cache is disposed. (Caches can get dispose’d if the app wants it that way, even during lifecycle of the app). We want to unregister them so that 1) do not leak the cache reference 2) unnecessarily do redundant operation of setNodeCoherent(true)/dispose() on an already disposed cache. Before registering/unregistering from the shutdown hook, thrs a check to see if shutdown is already in progress or not and throws exception if shutdown is already in progress.

Now changing the shutdown hook to call dispose() instead of setNodeCoherent(true) creates a cycle. Stack trace is: java.lang.IllegalStateException: Shutdown in progress at org.terracotta.modules.ehcache.coherence.CacheShutdownHook.checkIfShutdownInProgress(CacheShutdownHook.java:58) at org.terracotta.modules.ehcache.coherence.CacheShutdownHook.unregisterCache(CacheShutdownHook.java:78) at org.terracotta.modules.ehcache.store.ClusteredStore.dispose(ClusteredStore.java:495) at net.sf.ehcache.Cache.dispose(Cache.java:1996) at org.terracotta.modules.ehcache.coherence.CacheShutdownHook.shutdownRegisteredCaches(CacheShutdownHook.java:52) at org.terracotta.modules.ehcache.coherence.CacheShutdownHook.access$000(CacheShutdownHook.java:18) at org.terracotta.modules.ehcache.coherence.CacheShutdownHook$1.run(CacheShutdownHook.java:30) at com.tc.object.ClientShutdownManager.executeBeforeShutdownHooks(ClientShutdownManager.java:61)

After change, shutdown hook calls cache.dispose() which then tries to unregister the cache from the shutdown hook itself. And the check gets triggered that shutdown is already in progress.

Am not so keen on fixing it up. Things are working fine smoothly without this change :) The main motive was the idea that setNodeCoherent() may be asynchronous some day. I think we can deal with this when we get there.