diff options
5 files changed, 371 insertions, 16 deletions
diff --git a/policy-core/pom.xml b/policy-core/pom.xml index b6d394c3..cc41b215 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 c2d58b85..9bcf2598 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 @@ package org.onap.policy.drools.core.lock; 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 @@ public class SimpleLockManager { 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 @@ public class SimpleLockManager { 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 @@ public class SimpleLockManager { 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 @@ public class SimpleLockManager { 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 @@ public class SimpleLockManager { 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 2dd90f00..7ac9e31c 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 @@ package org.onap.policy.drools.core.lock; 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 fe883acf..26732e42 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.assertEquals; 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 833e1c7d..9aeba53c 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 @@ package org.onap.policy.drools.core.lock; 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.ExecutionException; 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 @@ public class SimpleLockManagerTest { 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 @@ public class SimpleLockManagerTest { } @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 @@ public class SimpleLockManagerTest { // 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 { |