Added expiration timer to default locking strategy
Fixed comment (missing "}").
Changed default age from 15 hours to 15 minutes.
Only increment test time by 1 when testing hashCode.
Change-Id: I70ef6cfd34e31ded45b3975f33e5b0ba22afa627
Issue-ID: POLICY-912
Signed-off-by: Jim Hahn <jrh3@att.com>
diff --git a/policy-core/pom.xml b/policy-core/pom.xml
index b6d394c..cc41b21 100644
--- a/policy-core/pom.xml
+++ b/policy-core/pom.xml
@@ -30,6 +30,11 @@
<version>1.3.0-SNAPSHOT</version>
</parent>
+ <properties>
+ <powermock.version>1.6.6</powermock.version>
+ </properties>
+
+
<dependencies>
<!--
Issue: 1 of 2
@@ -100,9 +105,15 @@
<scope>test</scope>
</dependency>
<dependency>
- <groupId>org.mockito</groupId>
- <artifactId>mockito-core</artifactId>
- <version>2.13.0</version>
+ <groupId>org.powermock</groupId>
+ <artifactId>powermock-api-mockito</artifactId>
+ <version>${powermock.version}</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.onap.policy.common</groupId>
+ <artifactId>utils-test</artifactId>
+ <version>${project.version}</version>
<scope>test</scope>
</dependency>
</dependencies>
diff --git a/policy-core/src/main/java/org/onap/policy/drools/core/lock/SimpleLockManager.java b/policy-core/src/main/java/org/onap/policy/drools/core/lock/SimpleLockManager.java
index c2d58b8..9bcf259 100644
--- a/policy-core/src/main/java/org/onap/policy/drools/core/lock/SimpleLockManager.java
+++ b/policy-core/src/main/java/org/onap/policy/drools/core/lock/SimpleLockManager.java
@@ -23,8 +23,14 @@
import static org.onap.policy.drools.core.lock.LockRequestFuture.MSG_NULL_OWNER;
import static org.onap.policy.drools.core.lock.LockRequestFuture.MSG_NULL_RESOURCE_ID;
import static org.onap.policy.drools.core.lock.LockRequestFuture.makeNullArgException;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.Map;
+import java.util.SortedSet;
+import java.util.TreeSet;
import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import org.onap.policy.common.utils.time.CurrentTime;
import org.onap.policy.drools.core.lock.PolicyResourceLockFeatureAPI.Callback;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -38,9 +44,33 @@
protected static Logger logger = LoggerFactory.getLogger(SimpleLockManager.class);
/**
- * Maps a resource to the owner that holds the lock on it.
+ * Maximum age, in milliseconds, before a lock is considered stale and released.
*/
- private ConcurrentHashMap<String, String> resource2owner = new ConcurrentHashMap<>();
+ protected static final long MAX_AGE_MS = TimeUnit.MINUTES.toMillis(15L);
+
+ /**
+ * Used to access the current time. May be overridden by junit tests.
+ */
+ private static CurrentTime currentTime = new CurrentTime();
+
+ /**
+ * Used to synchronize updates to {@link #resource2data} and {@link #locks}.
+ */
+ private final Object locker = new Object();
+
+ /**
+ * Maps a resource to its lock data. Lock data is stored in both this and in
+ * {@link #locks}.
+ */
+ private final Map<String, Data> resource2data = new HashMap<>();
+
+ /**
+ * Lock data, sorted by expiration time. Lock data is stored in both this and in
+ * {@link #resource2data}. Whenever a lock operation is performed, this structure is
+ * examined and any expired locks are removed; thus no timer threads are needed to
+ * remove expired locks.
+ */
+ private final SortedSet<Data> locks = new TreeSet<>();
/**
*
@@ -84,9 +114,20 @@
throw makeNullArgException(MSG_NULL_OWNER);
}
- boolean locked = (resource2owner.putIfAbsent(resourceId, owner) == null);
+ Data existingLock;
+
+ synchronized(locker) {
+ cleanUpLocks();
+
+ if((existingLock = resource2data.get(resourceId)) == null) {
+ Data data = new Data(owner, resourceId, currentTime.getMillis() + MAX_AGE_MS);
+ resource2data.put(resourceId, data);
+ locks.add(data);
+ }
+ }
- if (!locked && owner.equals(resource2owner.get(resourceId))) {
+ boolean locked = (existingLock == null);
+ if (existingLock != null && owner.equals(existingLock.getOwner())) {
throw new IllegalStateException("lock for resource " + resourceId + " already owned by " + owner);
}
@@ -112,8 +153,24 @@
if (owner == null) {
throw makeNullArgException(MSG_NULL_OWNER);
}
+
+ Data data;
+
+ synchronized(locker) {
+ cleanUpLocks();
+
+ if((data = resource2data.get(resourceId)) != null) {
+ if(owner.equals(data.getOwner())) {
+ resource2data.remove(resourceId);
+ locks.remove(data);
+
+ } else {
+ data = null;
+ }
+ }
+ }
- boolean unlocked = resource2owner.remove(resourceId, owner);
+ boolean unlocked = (data != null);
logger.info("unlock resource {} owner {} = {}", resourceId, owner, unlocked);
return unlocked;
@@ -132,7 +189,13 @@
throw makeNullArgException(MSG_NULL_RESOURCE_ID);
}
- boolean locked = resource2owner.containsKey(resourceId);
+ boolean locked;
+
+ synchronized(locker) {
+ cleanUpLocks();
+
+ locked = resource2data.containsKey(resourceId);
+ }
logger.debug("resource {} isLocked = {}", resourceId, locked);
@@ -157,9 +220,128 @@
throw makeNullArgException(MSG_NULL_OWNER);
}
- boolean locked = owner.equals(resource2owner.get(resourceId));
+ Data data;
+
+ synchronized(locker) {
+ cleanUpLocks();
+
+ data = resource2data.get(resourceId);
+ }
+
+ boolean locked = (data != null && owner.equals(data.getOwner()));
logger.debug("resource {} isLockedBy {} = {}", resourceId, owner, locked);
return locked;
}
+
+ /**
+ * Releases expired locks.
+ */
+ private void cleanUpLocks() {
+ long tcur = currentTime.getMillis();
+
+ synchronized(locker) {
+ Iterator<Data> it = locks.iterator();
+ while(it.hasNext()) {
+ Data d = it.next();
+ if(d.getExpirationMs() <= tcur) {
+ it.remove();
+ resource2data.remove(d.getResource());
+
+ } else {
+ break;
+ }
+ }
+ }
+ }
+
+ /**
+ * Data for a single Lock. Sorts by expiration time, then resource, and
+ * then owner.
+ */
+ protected static class Data implements Comparable<Data> {
+
+ /**
+ * Owner of the lock.
+ */
+ private final String owner;
+
+ /**
+ * Resource that is locked.
+ */
+ private final String resource;
+
+ /**
+ * Time when the lock will expire, in milliseconds.
+ */
+ private final long texpireMs;
+
+ /**
+ *
+ * @param resource
+ * @param owner
+ * @param texpireMs
+ */
+ public Data(String owner, String resource, long texpireMs) {
+ this.owner = owner;
+ this.resource = resource;
+ this.texpireMs = texpireMs;
+ }
+
+ public String getOwner() {
+ return owner;
+ }
+
+ public String getResource() {
+ return resource;
+ }
+
+ public long getExpirationMs() {
+ return texpireMs;
+ }
+
+ @Override
+ public int compareTo(Data o) {
+ int diff = Long.compare(texpireMs, o.texpireMs);
+ if(diff == 0)
+ diff = resource.compareTo(o.resource);
+ if(diff == 0)
+ diff = owner.compareTo(o.owner);
+ return diff;
+ }
+
+ @Override
+ public int hashCode() {
+ final int prime = 31;
+ int result = 1;
+ result = prime * result + ((owner == null) ? 0 : owner.hashCode());
+ result = prime * result + ((resource == null) ? 0 : resource.hashCode());
+ result = prime * result + (int) (texpireMs ^ (texpireMs >>> 32));
+ return result;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj)
+ return true;
+ if (obj == null)
+ return false;
+ if (getClass() != obj.getClass())
+ return false;
+ Data other = (Data) obj;
+ if (owner == null) {
+ if (other.owner != null)
+ return false;
+ } else if (!owner.equals(other.owner))
+ return false;
+ if (resource == null) {
+ if (other.resource != null)
+ return false;
+ } else if (!resource.equals(other.resource))
+ return false;
+ if (texpireMs != other.texpireMs)
+ return false;
+ return true;
+ }
+ }
}
diff --git a/policy-core/src/test/java/org/onap/policy/drools/core/lock/LockRequestFutureTest.java b/policy-core/src/test/java/org/onap/policy/drools/core/lock/LockRequestFutureTest.java
index 2dd90f0..7ac9e31 100644
--- a/policy-core/src/test/java/org/onap/policy/drools/core/lock/LockRequestFutureTest.java
+++ b/policy-core/src/test/java/org/onap/policy/drools/core/lock/LockRequestFutureTest.java
@@ -23,7 +23,7 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
-import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.Matchers.anyBoolean;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
diff --git a/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockManagerTest.java b/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockManagerTest.java
index fe883ac..26732e4 100644
--- a/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockManagerTest.java
+++ b/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockManagerTest.java
@@ -24,9 +24,9 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
-import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.ArgumentMatchers.anyBoolean;
-import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anyBoolean;
+import static org.mockito.Matchers.anyString;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
diff --git a/policy-core/src/test/java/org/onap/policy/drools/core/lock/SimpleLockManagerTest.java b/policy-core/src/test/java/org/onap/policy/drools/core/lock/SimpleLockManagerTest.java
index 833e1c7..9aeba53 100644
--- a/policy-core/src/test/java/org/onap/policy/drools/core/lock/SimpleLockManagerTest.java
+++ b/policy-core/src/test/java/org/onap/policy/drools/core/lock/SimpleLockManagerTest.java
@@ -22,6 +22,7 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import static org.onap.policy.drools.core.lock.TestUtils.expectException;
import java.util.LinkedList;
@@ -31,8 +32,14 @@
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
+import org.junit.AfterClass;
import org.junit.Before;
+import org.junit.BeforeClass;
import org.junit.Test;
+import org.onap.policy.common.utils.time.CurrentTime;
+import org.onap.policy.common.utils.time.TestTime;
+import org.onap.policy.drools.core.lock.SimpleLockManager.Data;
+import org.powermock.reflect.Whitebox;
public class SimpleLockManagerTest {
@@ -42,18 +49,45 @@
private static final String RESOURCE_A = "resource.a";
private static final String RESOURCE_B = "resource.b";
private static final String RESOURCE_C = "resource.c";
+ private static final String RESOURCE_D = "resource.d";
private static final String OWNER1 = "owner.one";
private static final String OWNER2 = "owner.two";
private static final String OWNER3 = "owner.three";
+
+ /**
+ * Name of the "current time" field within the {@link SimpleLockManager} class.
+ */
+ private static final String TIME_FIELD = "currentTime";
+
+ private static CurrentTime savedTime;
+ private TestTime testTime;
private Future<Boolean> fut;
private SimpleLockManager mgr;
+
+ @BeforeClass
+ public static void setUpBeforeClass() {
+ savedTime = Whitebox.getInternalState(SimpleLockManager.class, TIME_FIELD);
+ }
+
+ @AfterClass
+ public static void tearDownAfterClass() {
+ Whitebox.setInternalState(SimpleLockManager.class, TIME_FIELD, savedTime);
+ }
@Before
public void setUp() {
+ testTime = new TestTime();
+ Whitebox.setInternalState(SimpleLockManager.class, TIME_FIELD, testTime);
+
mgr = new SimpleLockManager();
}
+
+ @Test
+ public void testCurrentTime() {
+ assertNotNull(savedTime);
+ }
@Test
public void testLock() throws Exception {
@@ -131,6 +165,12 @@
}
@Test
+ public void testUnlock_DiffOwner() {
+ mgr.lock(RESOURCE_A, OWNER1, null);
+ assertFalse(mgr.unlock(RESOURCE_A, OWNER2));
+ }
+
+ @Test
public void testIsLocked() {
assertFalse(mgr.isLocked(RESOURCE_A));
@@ -203,6 +243,128 @@
// different owner
assertFalse(mgr.isLockedBy(RESOURCE_A, OWNER2));
}
+
+ @Test
+ public void testCleanUpLocks() throws Exception {
+ // note: this assumes that MAX_AGE_MS is divisible by 4
+ mgr.lock(RESOURCE_A, OWNER1, null);
+ assertTrue(mgr.isLocked(RESOURCE_A));
+
+ testTime.sleep(10);
+ mgr.lock(RESOURCE_B, OWNER1, null);
+ assertTrue(mgr.isLocked(RESOURCE_A));
+ assertTrue(mgr.isLocked(RESOURCE_B));
+
+ testTime.sleep(SimpleLockManager.MAX_AGE_MS/4);
+ mgr.lock(RESOURCE_C, OWNER1, null);
+ assertTrue(mgr.isLocked(RESOURCE_A));
+ assertTrue(mgr.isLocked(RESOURCE_B));
+ assertTrue(mgr.isLocked(RESOURCE_C));
+
+ testTime.sleep(SimpleLockManager.MAX_AGE_MS/4);
+ mgr.lock(RESOURCE_D, OWNER1, null);
+ assertTrue(mgr.isLocked(RESOURCE_A));
+ assertTrue(mgr.isLocked(RESOURCE_B));
+ assertTrue(mgr.isLocked(RESOURCE_C));
+ assertTrue(mgr.isLocked(RESOURCE_D));
+
+ // sleep remainder of max age - first two should expire
+ testTime.sleep(SimpleLockManager.MAX_AGE_MS/2);
+ assertFalse(mgr.isLocked(RESOURCE_A));
+ assertFalse(mgr.isLocked(RESOURCE_B));
+ assertTrue(mgr.isLocked(RESOURCE_C));
+ assertTrue(mgr.isLocked(RESOURCE_D));
+
+ // another quarter - next one should expire
+ testTime.sleep(SimpleLockManager.MAX_AGE_MS/4);
+ assertFalse(mgr.isLocked(RESOURCE_C));
+ assertTrue(mgr.isLocked(RESOURCE_D));
+
+ // another quarter - last one should expire
+ testTime.sleep(SimpleLockManager.MAX_AGE_MS/4);
+ assertFalse(mgr.isLocked(RESOURCE_D));
+ }
+
+ @Test
+ public void testDataGetXxx() {
+ long ttime = System.currentTimeMillis() + 5;
+ Data data = new Data(OWNER1, RESOURCE_A, ttime);
+
+ assertEquals(OWNER1, data.getOwner());
+ assertEquals(RESOURCE_A, data.getResource());
+ assertEquals(ttime, data.getExpirationMs());
+ }
+
+ @Test
+ public void testDataCompareTo() {
+ long ttime = System.currentTimeMillis() + 50;
+ Data data = new Data(OWNER1, RESOURCE_A, ttime);
+ Data dataSame = new Data(OWNER1, RESOURCE_A, ttime);
+ Data dataDiffExpire = new Data(OWNER1, RESOURCE_A, ttime+1);
+ Data dataDiffOwner = new Data(OWNER2, RESOURCE_A, ttime);
+ Data dataDiffResource = new Data(OWNER1, RESOURCE_B, ttime);
+
+ assertEquals(0, data.compareTo(data));
+ assertEquals(0, data.compareTo(dataSame));
+
+ assertTrue(data.compareTo(dataDiffExpire) < 0);
+ assertTrue(dataDiffExpire.compareTo(data) > 0);
+
+ assertTrue(data.compareTo(dataDiffOwner) < 0);
+ assertTrue(dataDiffOwner.compareTo(data) > 0);
+
+ assertTrue(data.compareTo(dataDiffResource) < 0);
+ assertTrue(dataDiffResource.compareTo(data) > 0);
+ }
+
+ @Test
+ public void testDataHashCode() {
+ long ttime = System.currentTimeMillis() + 1;
+ Data data = new Data(OWNER1, RESOURCE_A, ttime);
+ Data dataSame = new Data(OWNER1, RESOURCE_A, ttime);
+ Data dataDiffExpire = new Data(OWNER1, RESOURCE_A, ttime+1);
+ Data dataDiffOwner = new Data(OWNER2, RESOURCE_A, ttime);
+ Data dataDiffResource = new Data(OWNER1, RESOURCE_B, ttime);
+ Data dataNullOwner = new Data(null, RESOURCE_A, ttime);
+ Data dataNullResource = new Data(OWNER1, null, ttime);
+
+ int hc1 = data.hashCode();
+ assertEquals(hc1, dataSame.hashCode());
+
+ assertTrue(hc1 != dataDiffExpire.hashCode());
+ assertTrue(hc1 != dataDiffOwner.hashCode());
+ assertTrue(hc1 != dataDiffResource.hashCode());
+ assertTrue(hc1 != dataNullOwner.hashCode());
+ assertTrue(hc1 != dataNullResource.hashCode());
+ }
+
+ @Test
+ public void testDataEquals() {
+ long ttime = System.currentTimeMillis() + 50;
+ Data data = new Data(OWNER1, RESOURCE_A, ttime);
+ Data dataSame = new Data(OWNER1, RESOURCE_A, ttime);
+ Data dataDiffExpire = new Data(OWNER1, RESOURCE_A, ttime+1);
+ Data dataDiffOwner = new Data(OWNER2, RESOURCE_A, ttime);
+ Data dataDiffResource = new Data(OWNER1, RESOURCE_B, ttime);
+ Data dataNullOwner = new Data(null, RESOURCE_A, ttime);
+ Data dataNullResource = new Data(OWNER1, null, ttime);
+
+ assertTrue(data.equals(data));
+ assertTrue(data.equals(dataSame));
+
+ assertFalse(data.equals(dataDiffExpire));
+ assertFalse(data.equals(dataDiffOwner));
+ assertFalse(data.equals(dataDiffResource));
+
+ assertFalse(data.equals(null));
+ assertFalse(data.equals("string"));
+
+ assertFalse(dataNullOwner.equals(data));
+ assertFalse(dataNullResource.equals(data));
+
+ assertTrue(dataNullOwner.equals(new Data(null, RESOURCE_A, ttime)));
+ assertTrue(dataNullResource.equals(new Data(OWNER1, null, ttime)));
+ }
@Test
public void testMultiThreaded() throws InterruptedException {