[jbosscache-commits] JBoss Cache SVN: r5468 - in core/trunk/src: main/java/org/jboss/cache/factories and 4 other directories.
jbosscache-commits at lists.jboss.org
jbosscache-commits at lists.jboss.org
Thu Mar 27 10:34:38 EDT 2008
Author: manik.surtani at jboss.com
Date: 2008-03-27 10:34:37 -0400 (Thu, 27 Mar 2008)
New Revision: 5468
Added:
core/trunk/src/main/java/org/jboss/cache/factories/annotations/Destroy.java
Modified:
core/trunk/src/main/java/org/jboss/cache/Cache.java
core/trunk/src/main/java/org/jboss/cache/CacheImpl.java
core/trunk/src/main/java/org/jboss/cache/RegionManager.java
core/trunk/src/main/java/org/jboss/cache/factories/ComponentRegistry.java
core/trunk/src/main/java/org/jboss/cache/invocation/AbstractInvocationDelegate.java
core/trunk/src/main/java/org/jboss/cache/invocation/CacheInvocationDelegate.java
core/trunk/src/main/java/org/jboss/cache/notifications/Notifier.java
core/trunk/src/test/java/org/jboss/cache/api/DestroyedCacheAPITest.java
Log:
JBCACHE-1312 - NPE from Cache methods after destroy() is called
Modified: core/trunk/src/main/java/org/jboss/cache/Cache.java
===================================================================
--- core/trunk/src/main/java/org/jboss/cache/Cache.java 2008-03-27 13:00:06 UTC (rev 5467)
+++ core/trunk/src/main/java/org/jboss/cache/Cache.java 2008-03-27 14:34:37 UTC (rev 5468)
@@ -359,8 +359,7 @@
/**
* @return the current invocation context for the current invocation and cache instance.
- * @throws IllegalStateException if {@link #getCacheStatus()} would not return {@link CacheStatus#STARTED}.
- * @see org.jboss.cache.InvocationContext
+ * @throws IllegalStateException if the cache has been destroyed.
*/
InvocationContext getInvocationContext();
@@ -368,7 +367,7 @@
* Sets the passed in {@link org.jboss.cache.InvocationContext} as current.
*
* @param ctx invocation context to use
- * @throws IllegalStateException if {@link #getCacheStatus()} would not return {@link CacheStatus#STARTED}.
+ * @throws IllegalStateException if the cache has been destroyed.
*/
void setInvocationContext(InvocationContext ctx);
Modified: core/trunk/src/main/java/org/jboss/cache/CacheImpl.java
===================================================================
--- core/trunk/src/main/java/org/jboss/cache/CacheImpl.java 2008-03-27 13:00:06 UTC (rev 5467)
+++ core/trunk/src/main/java/org/jboss/cache/CacheImpl.java 2008-03-27 14:34:37 UTC (rev 5468)
@@ -523,9 +523,7 @@
private void internalDestroy()
{
cacheStatus = CacheStatus.DESTROYING;
- regionManager = null;
cacheLoaderManager = null;
- notifier = null;
// The rest of these should have already been taken care of in stop,
// but we do it here as well in case stop failed.
Modified: core/trunk/src/main/java/org/jboss/cache/RegionManager.java
===================================================================
--- core/trunk/src/main/java/org/jboss/cache/RegionManager.java 2008-03-27 13:00:06 UTC (rev 5467)
+++ core/trunk/src/main/java/org/jboss/cache/RegionManager.java 2008-03-27 14:34:37 UTC (rev 5468)
@@ -12,6 +12,7 @@
import org.jboss.cache.config.EvictionRegionConfig;
import org.jboss.cache.eviction.EvictionTimerTask;
import org.jboss.cache.eviction.RegionNameConflictException;
+import org.jboss.cache.factories.annotations.Destroy;
import org.jboss.cache.factories.annotations.Inject;
import org.jboss.cache.factories.annotations.Start;
import org.jboss.cache.factories.annotations.Stop;
@@ -98,6 +99,13 @@
if (isUsingEvictions()) stopEvictionThread();
}
+ @Destroy
+ protected void destroy()
+ {
+ regionsRegistry.clear();
+ activationChangeNodes.clear();
+ }
+
/**
* @return true if evictions are being processed.
*/
Modified: core/trunk/src/main/java/org/jboss/cache/factories/ComponentRegistry.java
===================================================================
--- core/trunk/src/main/java/org/jboss/cache/factories/ComponentRegistry.java 2008-03-27 13:00:06 UTC (rev 5467)
+++ core/trunk/src/main/java/org/jboss/cache/factories/ComponentRegistry.java 2008-03-27 14:34:37 UTC (rev 5468)
@@ -12,6 +12,7 @@
import org.jboss.cache.factories.annotations.CacheInjectionMethods;
import org.jboss.cache.factories.annotations.ComponentName;
import org.jboss.cache.factories.annotations.DefaultFactoryFor;
+import org.jboss.cache.factories.annotations.Destroy;
import org.jboss.cache.factories.annotations.Inject;
import org.jboss.cache.factories.annotations.Start;
import org.jboss.cache.factories.annotations.Stop;
@@ -629,15 +630,21 @@
Component conf = componentLookup.get(Configuration.class.getName());
Component cr = componentLookup.get(ComponentRegistry.class.getName());
+ // destroy all components to clean up resources
+ for (Component c : componentLookup.values()) c.changeState(DESTROYED);
+
+ // remove from componentLookup
componentLookup.clear();
+ // bootstrap components
deployerClassLoader.changeState(CONSTRUCTED);
spi.changeState(CONSTRUCTED);
impl.changeState(CONSTRUCTED);
conf.changeState(CONSTRUCTED);
cr.changeState(CONSTRUCTED);
- bootstrap = new Bootstrap((ClassLoader) deployerClassLoader.instance, (CacheImpl) impl.instance, (CacheSPI) spi.instance, (ComponentRegistry) cr.instance, (Configuration) conf.instance);
+ bootstrap = new Bootstrap((ClassLoader) deployerClassLoader.instance, (CacheImpl) impl.instance, (CacheSPI) spi.instance,
+ (ComponentRegistry) cr.instance, (Configuration) conf.instance);
overallState = null;
}
@@ -851,6 +858,9 @@
case STARTED:
start();
break;
+ case DESTROYED:
+ destroy();
+ break;
case CONSTRUCTED:
// nothing to do here.
}
@@ -915,6 +925,14 @@
invokeMethods(Stop.class);
}
+ /**
+ * Used to call all methods annotated with {@link org.jboss.cache.factories.annotations.Destroy} on component instance
+ */
+ void destroy()
+ {
+ invokeMethods(Destroy.class);
+ }
+
private void invokeMethods(Class<? extends Annotation> annotation)
{
List<Method> methods = ReflectionUtil.getAllMethods(instance.getClass(), annotation);
Copied: core/trunk/src/main/java/org/jboss/cache/factories/annotations/Destroy.java (from rev 5467, core/trunk/src/main/java/org/jboss/cache/factories/annotations/Stop.java)
===================================================================
--- core/trunk/src/main/java/org/jboss/cache/factories/annotations/Destroy.java (rev 0)
+++ core/trunk/src/main/java/org/jboss/cache/factories/annotations/Destroy.java 2008-03-27 14:34:37 UTC (rev 5468)
@@ -0,0 +1,20 @@
+package org.jboss.cache.factories.annotations;
+
+import static java.lang.annotation.ElementType.METHOD;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * Method level annotation that indicates a (no-param) method to be called on a component registered in the ComponentRegistry
+ * when the cache is destroyed.
+ * <p/>
+ *
+ * @author Manik Surtani (<a href="mailto:manik at jboss.org">manik at jboss.org</a>)
+ * @since 2.1.0
+ */
+ at Target(METHOD)
+ at Retention(RetentionPolicy.RUNTIME)
+public @interface Destroy
+{
+}
\ No newline at end of file
Modified: core/trunk/src/main/java/org/jboss/cache/invocation/AbstractInvocationDelegate.java
===================================================================
--- core/trunk/src/main/java/org/jboss/cache/invocation/AbstractInvocationDelegate.java 2008-03-27 13:00:06 UTC (rev 5467)
+++ core/trunk/src/main/java/org/jboss/cache/invocation/AbstractInvocationDelegate.java 2008-03-27 14:34:37 UTC (rev 5468)
@@ -77,7 +77,7 @@
return invoke(call, skipCacheStatusCheck, invocationContextContainer.get());
}
- private void assertIsConstructed()
+ protected void assertIsConstructed()
{
if (invocationContextContainer == null) throw new IllegalStateException("The cache has been destroyed!");
}
Modified: core/trunk/src/main/java/org/jboss/cache/invocation/CacheInvocationDelegate.java
===================================================================
--- core/trunk/src/main/java/org/jboss/cache/invocation/CacheInvocationDelegate.java 2008-03-27 13:00:06 UTC (rev 5467)
+++ core/trunk/src/main/java/org/jboss/cache/invocation/CacheInvocationDelegate.java 2008-03-27 14:34:37 UTC (rev 5468)
@@ -75,12 +75,10 @@
{
this.stateTransferManager = null;
this.cacheLoaderManager = null;
- this.notifier = null;
this.transactionManager = null;
this.buddyManager = null;
this.transactionTable = null;
this.rpcManager = null;
- this.regionManager = null;
this.marshaller = null;
}
@@ -274,13 +272,14 @@
public InvocationContext getInvocationContext()
{
- if (!getCacheStatus().allowInvocations()) throw new IllegalStateException("Cache not in STARTED state!");
+
+ assertIsConstructed();
return invocationContextContainer.get();
}
public void setInvocationContext(InvocationContext ctx)
{
- if (!getCacheStatus().allowInvocations()) throw new IllegalStateException("Cache not in STARTED state!");
+ assertIsConstructed();
// assume a null ctx is meant to "un-set" the context?
if (ctx == null) invocationContextContainer.remove();
@@ -289,11 +288,13 @@
public Address getLocalAddress()
{
+ if (rpcManager == null) return null;
return rpcManager.getLocalAddress();
}
public List<Address> getMembers()
{
+ if (rpcManager == null) return null;
return rpcManager.getMembers();
}
@@ -325,6 +326,7 @@
public void evict(Fqn<?> fqn, boolean recursive)
{
+ if (!getCacheStatus().allowInvocations()) throw new IllegalStateException("Cache is not in STARTED state");
//this method should be called by eviction thread only, so no transaction - expected (sec param is false)
Node<K, V> node = peek(fqn, false);
if (node != null && node.isResident())
Modified: core/trunk/src/main/java/org/jboss/cache/notifications/Notifier.java
===================================================================
--- core/trunk/src/main/java/org/jboss/cache/notifications/Notifier.java 2008-03-27 13:00:06 UTC (rev 5467)
+++ core/trunk/src/main/java/org/jboss/cache/notifications/Notifier.java 2008-03-27 14:34:37 UTC (rev 5468)
@@ -14,6 +14,7 @@
import org.jboss.cache.Fqn;
import org.jboss.cache.InvocationContext;
import org.jboss.cache.buddyreplication.BuddyGroup;
+import org.jboss.cache.factories.annotations.Destroy;
import org.jboss.cache.factories.annotations.Inject;
import org.jboss.cache.marshall.MarshalledValueMap;
import org.jboss.cache.notifications.annotation.*;
@@ -63,15 +64,21 @@
{
}
+ public Notifier(Cache cache)
+ {
+ this.cache = cache;
+ }
+
@Inject
private void injectDependencies(CacheSPI cache)
{
this.cache = cache;
}
- public Notifier(Cache cache)
+ @Destroy
+ protected void destroy()
{
- this.cache = cache;
+ listenerInvocations.clear();
}
/**
Modified: core/trunk/src/test/java/org/jboss/cache/api/DestroyedCacheAPITest.java
===================================================================
--- core/trunk/src/test/java/org/jboss/cache/api/DestroyedCacheAPITest.java 2008-03-27 13:00:06 UTC (rev 5467)
+++ core/trunk/src/test/java/org/jboss/cache/api/DestroyedCacheAPITest.java 2008-03-27 14:34:37 UTC (rev 5468)
@@ -1,15 +1,5 @@
package org.jboss.cache.api;
-import static org.testng.AssertJUnit.assertEquals;
-import static org.testng.AssertJUnit.assertFalse;
-import static org.testng.AssertJUnit.assertNotNull;
-import static org.testng.AssertJUnit.assertNull;
-import static org.testng.AssertJUnit.assertSame;
-import static org.testng.AssertJUnit.fail;
-
-import java.util.HashMap;
-import java.util.Map;
-
import org.jboss.cache.Cache;
import org.jboss.cache.CacheFactory;
import org.jboss.cache.CacheStatus;
@@ -22,9 +12,13 @@
import org.jboss.cache.notifications.annotation.NodeCreated;
import org.jboss.cache.notifications.event.Event;
import org.jboss.cache.transaction.GenericTransactionManagerLookup;
+import static org.testng.AssertJUnit.*;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
+import java.util.HashMap;
+import java.util.Map;
+
/**
* Tests aspects of the {@link org.jboss.cache.Cache} public API when
* destroy() has been called on the cache.
@@ -40,7 +34,7 @@
private Fqn parent = Fqn.fromString("/test/fqn");
private Fqn child = Fqn.fromString("/test/fqn/child");
private String version;
-
+
@BeforeMethod(alwaysRun = true)
public void setUp() throws Exception
{
@@ -69,7 +63,7 @@
// but we aren't started, so should be able to change them
c.setCacheMode(Configuration.CacheMode.REPL_SYNC);
assertEquals(Configuration.CacheMode.REPL_SYNC, c.getCacheMode());
-
+
// others are always changeable.
c.setLockAcquisitionTimeout(100);
assertEquals(100, c.getLockAcquisitionTimeout());
@@ -82,17 +76,34 @@
*/
public void testCacheListeners()
{
- assertEquals(0, cache.getCacheListeners().size());
+ try
+ {
+ cache.getCacheListeners();
+ }
+ catch (IllegalStateException good)
+ {
+ // expected
+ }
Object dummy = new Listener();
- cache.addCacheListener(dummy);
+ try
+ {
+ cache.addCacheListener(dummy);
+ }
+ catch (IllegalStateException good)
+ {
+ // expected
+ }
- assertEquals(1, cache.getCacheListeners().size());
-
- cache.removeCacheListener(dummy);
-
- assertEquals(0, cache.getCacheListeners().size());
+ try
+ {
+ cache.removeCacheListener(dummy);
+ }
+ catch (IllegalStateException good)
+ {
+ // expected
+ }
}
/**
@@ -134,13 +145,13 @@
/**
* Tests the basic gets, puts. Expectation is all will throw an
- * ISE.
- *
+ * ISE.
+ * <p/>
* BES 2008/03/22 -- This behavior is not actually documented. Maintainers
* shouldn't feel constrained from updating this test to match
- * agreed upon behavior changes; I'm just adding it so any changes to the
+ * agreed upon behavior changes; I'm just adding it so any changes to the
* current behavior will trigger failures and ensure that people are aware of
- * the change and agree that it's correct.
+ * the change and agree that it's correct.
*/
public void testConvenienceMethods()
{
@@ -153,89 +164,111 @@
cache.get(parent, key);
fail("Get key on destroyed cache did not throw ISE");
}
- catch (IllegalStateException good) {}
+ catch (IllegalStateException good)
+ {
+ }
try
{
cache.getNode(parent);
fail("Get node on destroyed cache did not throw ISE");
}
- catch (IllegalStateException good) {}
+ catch (IllegalStateException good)
+ {
+ }
try
{
cache.put(parent, key, value);
fail("Put key/value on destroyed cache did not throw ISE");
}
- catch (IllegalStateException good) {}
+ catch (IllegalStateException good)
+ {
+ }
try
{
cache.putForExternalRead(parent, key, value);
fail("Put for external read on destroyed cache did not throw ISE");
}
- catch (IllegalStateException good) {}
+ catch (IllegalStateException good)
+ {
+ }
try
{
cache.put(parent, data);
fail("Put Map on destroyed cache did not throw ISE");
}
- catch (IllegalStateException good) {}
-
+ catch (IllegalStateException good)
+ {
+ }
+
try
{
cache.move(child, Fqn.ROOT);
fail("Remove move on destroyed cache did not throw ISE");
}
- catch (IllegalStateException good) {}
-
+ catch (IllegalStateException good)
+ {
+ }
+
try
{
cache.getData(parent);
fail("getData on destroyed cache did not throw ISE");
}
- catch (IllegalStateException good) {}
-
+ catch (IllegalStateException good)
+ {
+ }
+
try
{
cache.getKeys(parent);
fail("getKeys on destroyed cache did not throw ISE");
}
- catch (IllegalStateException good) {}
-
+ catch (IllegalStateException good)
+ {
+ }
+
try
{
cache.clearData(parent);
fail("clearData on destroyed cache did not throw ISE");
}
- catch (IllegalStateException good) {}
+ catch (IllegalStateException good)
+ {
+ }
try
{
cache.remove(parent, key);
fail("Remove key on destroyed cache did not throw ISE");
}
- catch (IllegalStateException good) {}
+ catch (IllegalStateException good)
+ {
+ }
try
{
cache.removeNode(parent);
fail("Remove node on destroyed cache did not throw ISE");
}
- catch (IllegalStateException good) {}
+ catch (IllegalStateException good)
+ {
+ }
}
-
+
/**
- * Tests that Cache.getRoot() returns a node.
- *
+ * Tests that Cache.getRoot() returns a node.
+ * <p/>
* BES 2008/03/22 -- This behavior is not actually documented. Maintainers
* shouldn't feel constrained from updating this test to match
- * agreed upon behavior changes; I'm just adding it so any changes to the
+ * agreed upon behavior changes; I'm just adding it so any changes to the
* current behavior will trigger failures and ensure that people are aware of
- * the change and agree that it's correct.
- */
+ * the change and agree that it's correct.
+ */
public void testGetRoot()
{
Node root = cache.getRoot();
@@ -245,14 +278,14 @@
/**
- * Tests the basic node addition, existence check, get, remove operations.
- * Expectation is all will throw an ISE.
- *
+ * Tests the basic node addition, existence check, get, remove operations.
+ * Expectation is all will throw an ISE.
+ * <p/>
* BES 2008/03/22 -- This behavior is not actually documented. Maintainers
* shouldn't feel constrained from updating this test to match
- * agreed upon behavior changes; I'm just adding it so any changes to the
+ * agreed upon behavior changes; I'm just adding it so any changes to the
* current behavior will trigger failures and ensure that people are aware of
- * the change and agree that it's correct.
+ * the change and agree that it's correct.
*/
public void testNodeAPI()
{
@@ -262,93 +295,103 @@
root.addChild(parent);
fail("addChild on destroyed cache did not throw ISE");
}
- catch (IllegalStateException good) {}
-
+ catch (IllegalStateException good)
+ {
+ }
+
try
{
root.hasChild(parent);
fail("hasChild on destroyed cache did not throw ISE");
}
- catch (IllegalStateException good) {}
-
+ catch (IllegalStateException good)
+ {
+ }
+
try
{
root.getChild(parent);
fail("getChild on destroyed cache did not throw ISE");
}
- catch (IllegalStateException good) {}
-
+ catch (IllegalStateException good)
+ {
+ }
+
try
{
root.removeChild(parent);
fail("removeChild on destroyed cache did not throw ISE");
}
- catch (IllegalStateException good) {}
+ catch (IllegalStateException good)
+ {
+ }
}
/**
- * Tests the evict operation. Expectation is it will throw an ISE, but
- * a success is OK as well.
- *
- * BES 2008/03/22 -- This behavior is not actually documented. Maintainers
- * shouldn't feel constrained from updating this test to match
- * agreed upon behavior changes; I'm just adding it so any changes to the
- * current behavior will trigger failures and ensure that people are aware of
- * the change and agree that it's correct.
+ * Tests the evict operation. Expectation is it will throw an ISE.
*/
public void testEvict()
{
try
{
cache.evict(parent, false);
+ assert false : "Should throw ISE";
}
- catch (IllegalStateException ok) {}
-
+ catch (IllegalStateException ok)
+ {
+ }
+
try
{
cache.evict(child, false);
+ assert false : "Should throw ISE";
}
- catch (IllegalStateException ok) {}
+ catch (IllegalStateException ok)
+ {
+ }
try
{
cache.evict(parent, true);
+ assert false : "Should throw ISE";
}
- catch (IllegalStateException ok) {}
+ catch (IllegalStateException ok)
+ {
+ }
}
public void testGetRegion()
- {
+ {
assertNull(cache.getRegion(parent, false));
-
+
// The javadoc mentions "UnsupportedOperationException" although it's
// not clear about why it would be thrown.
assertNotNull(cache.getRegion(Fqn.ROOT, true));
}
-
+
public void testRemoveRegion()
{
assertFalse(cache.removeRegion(parent));
}
-
+
public void testGetLocalAddress()
{
assertEquals("CacheMode.LOCAL cache has no address", null, cache.getLocalAddress());
}
-
+
public void testGetMembers()
{
assertNull(cache.getMembers());
}
-
+
public void testGetCacheStatus()
{
assertEquals(CacheStatus.DESTROYED, cache.getCacheStatus());
}
-
+
/**
* BES 2008/03/22. I don't know what the correct behavior should be.
- * The test checks the call succeeds, which is one possibility and is
+ * The test checks the call succeeds, which is one possibility and is
* what the current impl does. Can be something else, just not NPE.
*/
public void testInvocationContext()
@@ -357,19 +400,19 @@
cache.setInvocationContext(ctx);
assertSame(ctx, cache.getInvocationContext());
}
-
+
public void testGetVersion()
{
assertEquals(version, cache.getVersion());
}
-
+
@CacheListener
public class Listener
{
@NodeCreated
public void nodeCreated(Event e)
- {
+ {
}
}
}
More information about the jbosscache-commits
mailing list