Deny subsequent lock()

This is the final step of separating the lock "refresh" operation
from the original "lock" operation.  This step entails rejecting
subsequent "lock" requests, even by the same owner, when a resource
is already locked; "refresh" should now be used, instead, to extend
a lock.
Modified comments to indicate that the lock can only be extended
using "refresh".

Change-Id: I406cf60c076dbce87afbd94fb301732359dbd2db
Issue-ID: POLICY-872
Signed-off-by: Jim Hahn <jrh3@att.com>

diff --git a/feature-distributed-locking/src/main/java/org/onap/policy/distributed/locking/TargetLock.java b/feature-distributed-locking/src/main/java/org/onap/policy/distributed/locking/TargetLock.java
index 1db3453..0652897 100644 (file)
--- a/feature-distributed-locking/src/main/java/org/onap/policy/distributed/locking/TargetLock.java
+++ b/feature-distributed-locking/src/main/java/org/onap/policy/distributed/locking/TargetLock.java
@@ -132,7 +132,7 @@ public class TargetLock {
                try (Connection conn = dataSource.getConnection();
 
                                PreparedStatement updateStatement = conn.prepareStatement(
-                                               "UPDATE pooling.locks SET host = ?, owner = ?, expirationTime = timestampadd(second, ?, now()) WHERE resourceId = ? AND (owner = ? OR expirationTime < now())");
+                                               "UPDATE pooling.locks SET host = ?, owner = ?, expirationTime = timestampadd(second, ?, now()) WHERE resourceId = ? AND expirationTime < now()");
 
                                PreparedStatement insertStatement = conn.prepareStatement(
                                                "INSERT INTO pooling.locks (resourceId, host, owner, expirationTime) values (?, ?, ?, timestampadd(second, ?, now()))");) {
@@ -142,7 +142,6 @@ public class TargetLock {
                        updateStatement.setString(i++, this.owner);
                        updateStatement.setInt(i++, holdSec);
             updateStatement.setString(i++, this.resourceId);
-            updateStatement.setString(i++, this.owner);
 
                        // The lock was expired and we grabbed it.
                        // return true

diff --git a/feature-distributed-locking/src/test/java/org/onap/policy/distributed/locking/TargetLockTest.java b/feature-distributed-locking/src/test/java/org/onap/policy/distributed/locking/TargetLockTest.java
index 6e33f22..42c8a74 100644 (file)
--- a/feature-distributed-locking/src/test/java/org/onap/policy/distributed/locking/TargetLockTest.java
+++ b/feature-distributed-locking/src/test/java/org/onap/policy/distributed/locking/TargetLockTest.java
@@ -92,8 +92,10 @@ public class TargetLockTest {
                
                assertEquals(OperResult.OPER_ACCEPTED, distLockFeat.beforeLock("resource1", "owner1", MAX_AGE_SEC));
 
-               // extend the lock
-        assertEquals(OperResult.OPER_ACCEPTED, distLockFeat.beforeLock("resource1", "owner1", MAX_AGE_SEC));
+               // cannot re-lock
+        assertEquals(OperResult.OPER_DENIED, distLockFeat.beforeLock("resource1", "owner1", MAX_AGE_SEC));
+
+        assertEquals(OperResult.OPER_ACCEPTED, distLockFeat.beforeIsLockedBy("resource1", "owner1"));
        }
 
        @Test
@@ -131,6 +133,8 @@ public class TargetLockTest {
         // refresh should work now
         assertEquals(OperResult.OPER_ACCEPTED, distLockFeat.beforeRefresh("resource1", "owner1", MAX_AGE_SEC));
         
+        assertEquals(OperResult.OPER_ACCEPTED, distLockFeat.beforeIsLockedBy("resource1", "owner1"));
+        
         // expire the lock
         try (PreparedStatement updateStatement = conn.prepareStatement("UPDATE pooling.locks SET expirationTime = timestampadd(second, -1, now()) WHERE resourceId = ?");)
         {
@@ -144,6 +148,8 @@ public class TargetLockTest {
         
         // refresh should fail now
         assertEquals(OperResult.OPER_DENIED, distLockFeat.beforeRefresh("resource1", "owner1", MAX_AGE_SEC));
+
+        assertEquals(OperResult.OPER_DENIED, distLockFeat.beforeIsLockedBy("resource1", "owner1"));
     }
        
        

diff --git a/policy-core/src/main/java/org/onap/policy/drools/core/lock/PolicyResourceLockFeatureAPI.java b/policy-core/src/main/java/org/onap/policy/drools/core/lock/PolicyResourceLockFeatureAPI.java
index ba6fe85..5aee490 100644 (file)
--- a/policy-core/src/main/java/org/onap/policy/drools/core/lock/PolicyResourceLockFeatureAPI.java
+++ b/policy-core/src/main/java/org/onap/policy/drools/core/lock/PolicyResourceLockFeatureAPI.java
@@ -71,8 +71,7 @@ public interface PolicyResourceLockFeatureAPI extends OrderedService {
     }
 
     /**
-     * This method is called before a lock is acquired on a resource.  It may be
-     * invoked repeatedly to extend the time that a lock is held.
+     * This method is called before a lock is acquired on a resource.
      * 
      * @param resourceId
      * @param owner

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 081ad4c..243ba3c 100644 (file)
--- 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
@@ -74,7 +74,9 @@ public class SimpleLockManager {
     }
 
     /**
-     * Attempts to lock a resource, extending the lock if the owner already holds it.
+     * Attempts to lock a resource, rejecting the lock if it is already owned, even if
+     * it's the same owner; the same owner can use {@link #refresh(String, String, int)
+     * refresh()}, instead, to extend a lock on a resource.
      * 
      * @param resourceId
      * @param owner
@@ -93,25 +95,19 @@ public class SimpleLockManager {
             throw makeNullArgException(MSG_NULL_OWNER);
         }
 
-        Data existingLock;
+        boolean locked = false;
         
         synchronized(locker) {
             cleanUpLocks();
 
-            if ((existingLock = resource2data.get(resourceId)) != null && existingLock.getOwner().equals(owner)) {
-                // replace the existing lock with an extended lock
-                locks.remove(existingLock);
-                existingLock = null;
-            }
-
-            if (existingLock == null) {
+            if(!resource2data.containsKey(resourceId)) {
                 Data data = new Data(owner, resourceId, currentTime.getMillis() + TimeUnit.SECONDS.toMillis(holdSec));
                 resource2data.put(resourceId, data);
                 locks.add(data);
+                locked = true;
             }
         }
 
-        boolean locked = (existingLock == null);
         logger.info("lock {} for resource {} owner {}", locked, resourceId, owner);
 
         return locked;

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 df6fb10..feabdbc 100644 (file)
--- 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
@@ -116,7 +116,7 @@ public class SimpleLockManagerTest {
         assertTrue(mgr.isLockedBy(RESOURCE_A, OWNER1));
         
         // extend the lock
-        mgr.lock(RESOURCE_A, OWNER1, MAX_AGE_SEC);
+        mgr.refresh(RESOURCE_A, OWNER1, MAX_AGE_SEC);
         
         // verify still locked after sleeping the other half of the cycle
         testTime.sleep(MAX_AGE_MS/2+1);
@@ -132,7 +132,7 @@ public class SimpleLockManagerTest {
         mgr.lock(RESOURCE_A, OWNER1, MAX_AGE_SEC);
         
         // same owner
-        assertTrue(mgr.lock(RESOURCE_A, OWNER1, MAX_AGE_SEC));
+        assertFalse(mgr.lock(RESOURCE_A, OWNER1, MAX_AGE_SEC));
 
         // different owner
         assertFalse(mgr.lock(RESOURCE_A, OWNER2, MAX_AGE_SEC));