From: Jim Hahn <jrh3@att.com>
Date: Mon, 25 Jun 2018 21:23:02 +0000 (-0400)
Subject: Added expiration timer to default locking strategy
X-Git-Tag: 1.3.0~43^2
X-Git-Url: https://gerrit.onap.org/r/gitweb?a=commitdiff_plain;h=d8a6909ef0bb3433265295107b1450122eac919a;p=policy%2Fdrools-pdp.git

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 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 {
@@ -130,6 +164,12 @@ public class SimpleLockManagerTest {
         assertFalse(mgr.unlock(RESOURCE_A, OWNER1));
     }
 
+    @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 {