Reimplement Lock API using Lock objects

Modified PolicyResourceLockManager to just return a feature, deferring
the lock() call/method to the feature, itself.  The manager was also
modified so that, if it can't find an enabled provider, it will return
a default provider, whose lock() methods always fail.  Once a feature
has been identified, the manager will cache it for use thereafter.

Modified the feature API to return lock objects and simplified the
interface to remove the beforeXxx and afterXxx methods.
Moved the unlock and refresh methods from the feature API into the
lock class, renaming them to free and extend, respectively.
Added a separate, feature-simple-locking project, which implements
a simple version of the locking feature, over a single JVM.
Extensively revised the distributed locking feature to fit in with
the new API.

Added support for persistence so that the various LockImpl classes
can be serialized and still function correctly when they are
deserialized back into new feature instances
Added default implementations of free & extend to LockImpl.
Modified API to take the ownerKey string, instead of the owner object.
Removed Extractor as unneeded - may add via another review, if still useful.

Updates per review comments:
- Updated licenses in feature-simple-locking
- Added beforeCreateLock & afterCreateLock to feature API
- Moved SimpleLockingFeature into policy-management so that it's
  always available
- Moved the executor service, "exsvc", into PolicyEngine
- Moved Extrator into policy-utils
- Changed Extractor logging level for exceptions
- Fixed feature sequence numbers
- Fixed mixing of seconds and milliseconds
- Renamed exsvc
- Modified to use property method with default value
- Configured scheduled executor
- Added suffix to Extractor.register()
- Eliminated Feature Api and tied lock manager into engine
- Added non-null checks to LockImpl parameters
- Added non-null checks to createLock() parameters
- Checked that lockManager is initialized

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

3 files changed, 979 insertions, 1 deletions

diff --git a/policy-management/src/test/java/org/onap/policy/drools/system/PolicyEngineManagerTest.java b/policy-management/src/test/java/org/onap/policy/drools/system/PolicyEngineManagerTest.java
index 5e0ead9d..fe1a2345 100644
--- a/policy-management/src/test/java/org/onap/policy/drools/system/PolicyEngineManagerTest.java
+++ b/policy-management/src/test/java/org/onap/policy/drools/system/PolicyEngineManagerTest.java
@@ -27,12 +27,14 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyLong;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -41,6 +43,7 @@ import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Properties;
+import java.util.concurrent.ScheduledExecutorService;
 import java.util.function.BiConsumer;
 import java.util.function.Consumer;
 import org.junit.Before;
@@ -53,6 +56,10 @@ import org.onap.policy.common.endpoints.http.server.HttpServletServer;
 import org.onap.policy.common.endpoints.http.server.HttpServletServerFactory;
 import org.onap.policy.common.utils.gson.GsonTestUtils;
 import org.onap.policy.drools.controller.DroolsController;
+import org.onap.policy.drools.core.lock.Lock;
+import org.onap.policy.drools.core.lock.LockCallback;
+import org.onap.policy.drools.core.lock.LockImpl;
+import org.onap.policy.drools.core.lock.PolicyResourceLockManager;
 import org.onap.policy.drools.features.PolicyControllerFeatureApi;
 import org.onap.policy.drools.features.PolicyEngineFeatureApi;
 import org.onap.policy.drools.persistence.SystemPersistence;
@@ -61,9 +68,10 @@ import org.onap.policy.drools.protocol.coders.EventProtocolCoder;
 import org.onap.policy.drools.protocol.configuration.ControllerConfiguration;
 import org.onap.policy.drools.protocol.configuration.DroolsConfiguration;
 import org.onap.policy.drools.protocol.configuration.PdpdConfiguration;
+import org.onap.policy.drools.system.internal.SimpleLockManager;
+import org.onap.policy.drools.system.internal.SimpleLockProperties;
 
 public class PolicyEngineManagerTest {
-
     private static final String EXPECTED = "expected exception";
 
     private static final String NOOP_STR = CommInfrastructure.NOOP.name();
@@ -76,6 +84,8 @@ public class PolicyEngineManagerTest {
     private static final String FEATURE2 = "feature-b";
     private static final String MY_TOPIC = "my-topic";
     private static final String MESSAGE = "my-message";
+    private static final String MY_OWNER = "my-owner";
+    private static final String MY_RESOURCE = "my-resource";
 
     private static final Object MY_EVENT = new Object();
 
@@ -125,6 +135,8 @@ public class PolicyEngineManagerTest {
     private PdpdConfiguration pdpConfig;
     private String pdpConfigJson;
     private PolicyEngineManager mgr;
+    private ScheduledExecutorService exsvc;
+    private PolicyResourceLockManager lockmgr;
 
     /**
      * Initializes the object to be tested.
@@ -176,6 +188,15 @@ public class PolicyEngineManagerTest {
         config3 = new ControllerConfiguration();
         config4 = new ControllerConfiguration();
         pdpConfig = new PdpdConfiguration();
+        exsvc = mock(ScheduledExecutorService.class);
+        lockmgr = mock(PolicyResourceLockManager.class);
+
+        when(lockmgr.start()).thenReturn(true);
+        when(lockmgr.stop()).thenReturn(true);
+        when(lockmgr.lock()).thenReturn(true);
+        when(lockmgr.unlock()).thenReturn(true);
+
+        when(prov2.beforeCreateLockManager(any(), any())).thenReturn(lockmgr);
 
         when(prov1.getName()).thenReturn(FEATURE1);
         when(prov2.getName()).thenReturn(FEATURE2);
@@ -387,6 +408,86 @@ public class PolicyEngineManagerTest {
         assertFalse(config.isEmpty());
     }
 
+    /**
+     * Tests that makeExecutorService() uses the value from the thread
+     * property.
+     */
+    @Test
+    public void testMakeExecutorServicePropertyProvided() {
+        PolicyEngineManager mgrspy = spy(mgr);
+
+        properties.setProperty(PolicyEngineManager.EXECUTOR_THREAD_PROP, "3");
+        mgrspy.configure(properties);
+        assertSame(exsvc, mgrspy.getExecutorService());
+        verify(mgrspy).makeScheduledExecutor(3);
+    }
+
+    /**
+     * Tests that makeExecutorService() uses the default thread count when no thread
+     * property is provided.
+     */
+    @Test
+    public void testMakeExecutorServiceNoProperty() {
+        PolicyEngineManager mgrspy = spy(mgr);
+
+        mgrspy.configure(properties);
+        assertSame(exsvc, mgrspy.getExecutorService());
+        verify(mgrspy).makeScheduledExecutor(PolicyEngineManager.DEFAULT_EXECUTOR_THREADS);
+    }
+
+    /**
+     * Tests that makeExecutorService() uses the default thread count when the thread
+     * property is invalid.
+     */
+    @Test
+    public void testMakeExecutorServiceInvalidProperty() {
+        PolicyEngineManager mgrspy = spy(mgr);
+
+        properties.setProperty(PolicyEngineManager.EXECUTOR_THREAD_PROP, "abc");
+        mgrspy.configure(properties);
+        assertSame(exsvc, mgrspy.getExecutorService());
+        verify(mgrspy).makeScheduledExecutor(PolicyEngineManager.DEFAULT_EXECUTOR_THREADS);
+    }
+
+    /**
+     * Tests createLockManager() when beforeCreateLock throws an exception and returns a
+     * manager.
+     */
+    @Test
+    public void testCreateLockManagerHaveProvider() {
+        // first provider throws an exception
+        when(prov1.beforeCreateLockManager(any(), any())).thenThrow(new RuntimeException(EXPECTED));
+
+        mgr.configure(properties);
+        assertSame(lockmgr, mgr.getLockManager());
+    }
+
+    /**
+     * Tests createLockManager() when SimpleLockManager throws an exception.
+     */
+    @Test
+    public void testCreateLockManagerSimpleEx() {
+        when(prov2.beforeCreateLockManager(any(), any())).thenReturn(null);
+
+        // invalid property for SimpleLockManager
+        properties.setProperty(SimpleLockProperties.EXPIRE_CHECK_SEC, "abc");
+        mgr.configure(properties);
+
+        // should create a manager using default properties
+        assertTrue(mgr.getLockManager() instanceof SimpleLockManager);
+    }
+
+    /**
+     * Tests createLockManager() when SimpleLockManager is returned.
+     */
+    @Test
+    public void testCreateLockManagerSimple() {
+        when(prov2.beforeCreateLockManager(any(), any())).thenReturn(null);
+
+        mgr.configure(properties);
+        assertTrue(mgr.getLockManager() instanceof SimpleLockManager);
+    }
+
     @Test
     public void testConfigureProperties() throws Exception {
         // arrange for first provider to throw exceptions
@@ -667,6 +768,12 @@ public class PolicyEngineManagerTest {
             when(sink1.start()).thenThrow(new RuntimeException(EXPECTED));
         });
 
+        // lock manager fails to start - still does everything
+        testStart(false, () -> when(lockmgr.start()).thenReturn(false));
+
+        //  lock manager throws an exception - still does everything
+        testStart(false, () -> when(lockmgr.start()).thenThrow(new RuntimeException(EXPECTED)));
+
         // servlet wait fails - still does everything
         testStart(false, () -> when(server1.waitedStart(anyLong())).thenReturn(false));
 
@@ -796,6 +903,12 @@ public class PolicyEngineManagerTest {
         // servlet fails to stop - still does everything
         testStop(false, () -> when(server1.stop()).thenReturn(false));
 
+        // lock manager fails to stop - still does everything
+        testStop(false, () -> when(lockmgr.stop()).thenReturn(false));
+
+        // lock manager throws an exception - still does everything
+        testStop(false, () -> when(lockmgr.stop()).thenThrow(new RuntimeException(EXPECTED)));
+
         // other tests
         checkBeforeAfter(
             (prov, flag) -> when(prov.beforeStop(mgr)).thenReturn(flag),
@@ -861,6 +974,10 @@ public class PolicyEngineManagerTest {
         assertTrue(threadStarted);
         assertTrue(threadInterrupted);
 
+
+        // lock manager throws an exception - still does everything
+        testShutdown(() -> doThrow(new RuntimeException(EXPECTED)).when(lockmgr).shutdown());
+
         // other tests
         checkBeforeAfter(
             (prov, flag) -> when(prov.beforeShutdown(mgr)).thenReturn(flag),
@@ -906,6 +1023,8 @@ public class PolicyEngineManagerTest {
 
         verify(prov1).afterShutdown(mgr);
         verify(prov2).afterShutdown(mgr);
+
+        verify(exsvc).shutdownNow();
     }
 
     @Test
@@ -985,6 +1104,12 @@ public class PolicyEngineManagerTest {
         // endpoint manager fails to lock - still does everything
         testLock(false, () -> when(endpoint.lock()).thenReturn(false));
 
+        // lock manager fails to lock - still does everything
+        testLock(false, () -> when(lockmgr.lock()).thenReturn(false));
+
+        // lock manager throws an exception - still does everything
+        testLock(false, () -> when(lockmgr.lock()).thenThrow(new RuntimeException(EXPECTED)));
+
         // other tests
         checkBeforeAfter(
             (prov, flag) -> when(prov.beforeLock(mgr)).thenReturn(flag),
@@ -1055,6 +1180,12 @@ public class PolicyEngineManagerTest {
         // endpoint manager fails to unlock - still does everything
         testUnlock(false, () -> when(endpoint.unlock()).thenReturn(false));
 
+        // lock manager fails to lock - still does everything
+        testUnlock(false, () -> when(lockmgr.unlock()).thenReturn(false));
+
+        // lock manager throws an exception - still does everything
+        testUnlock(false, () -> when(lockmgr.unlock()).thenThrow(new RuntimeException(EXPECTED)));
+
         // other tests
         checkBeforeAfter(
             (prov, flag) -> when(prov.beforeUnlock(mgr)).thenReturn(flag),
@@ -1484,6 +1615,32 @@ public class PolicyEngineManagerTest {
     }
 
     @Test
+    public void testCreateLock() {
+        Lock lock = mock(Lock.class);
+        LockCallback callback = mock(LockCallback.class);
+        when(lockmgr.createLock(MY_RESOURCE, MY_OWNER, 10, callback, false)).thenReturn(lock);
+
+        // not configured yet, thus no lock manager
+        assertThatIllegalStateException()
+                        .isThrownBy(() -> mgr.createLock(MY_RESOURCE, MY_OWNER, 10, callback, false));
+
+        // now configure it and try again
+        mgr.configure(properties);
+        assertSame(lock, mgr.createLock(MY_RESOURCE, MY_OWNER, 10, callback, false));
+
+        // test illegal args
+        assertThatThrownBy(() -> mgr.createLock(null, MY_OWNER, 10, callback, false))
+                        .hasMessageContaining("resourceId");
+        assertThatThrownBy(() -> mgr.createLock(MY_RESOURCE, null, 10, callback, false))
+                        .hasMessageContaining("ownerKey");
+        assertThatIllegalArgumentException()
+                        .isThrownBy(() -> mgr.createLock(MY_RESOURCE, MY_OWNER, -1, callback, false))
+                        .withMessageContaining("holdSec");
+        assertThatThrownBy(() -> mgr.createLock(MY_RESOURCE, MY_OWNER, 10, null, false))
+                        .hasMessageContaining("callback");
+    }
+
+    @Test
     public void testOpen() throws Throwable {
         when(prov1.beforeOpen(mgr)).thenThrow(new RuntimeException(EXPECTED));
         when(prov1.afterOpen(mgr)).thenThrow(new RuntimeException(EXPECTED));
@@ -1789,6 +1946,11 @@ public class PolicyEngineManagerTest {
             return engine;
         }
 
+        @Override
+        protected ScheduledExecutorService makeScheduledExecutor(int nthreads) {
+            return exsvc;
+        }
+
         /**
          * Shutdown thread with overrides.
          */
diff --git a/policy-management/src/test/java/org/onap/policy/drools/system/internal/SimpleLockManagerExceptionTest.java b/policy-management/src/test/java/org/onap/policy/drools/system/internal/SimpleLockManagerExceptionTest.java
new file mode 100644
index 00000000..7ffc72ff
--- /dev/null
+++ b/policy-management/src/test/java/org/onap/policy/drools/system/internal/SimpleLockManagerExceptionTest.java
@@ -0,0 +1,35 @@
+/*
+ * ============LICENSE_START=======================================================
+ * ONAP
+ * ================================================================================
+ * Copyright (C) 2019 AT&T Intellectual Property. All rights reserved.
+ * ================================================================================
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ * ============LICENSE_END=========================================================
+ */
+
+package org.onap.policy.drools.system.internal;
+
+import static org.junit.Assert.assertEquals;
+
+import org.junit.Test;
+import org.onap.policy.common.utils.test.ExceptionsTester;
+import org.onap.policy.drools.system.internal.SimpleLockManagerException;
+
+public class SimpleLockManagerExceptionTest extends ExceptionsTester {
+
+    @Test
+    public void test() {
+        assertEquals(1, test(SimpleLockManagerException.class));
+    }
+}
diff --git a/policy-management/src/test/java/org/onap/policy/drools/system/internal/SimpleLockManagerTest.java b/policy-management/src/test/java/org/onap/policy/drools/system/internal/SimpleLockManagerTest.java
new file mode 100644
index 00000000..66406898
--- /dev/null
+++ b/policy-management/src/test/java/org/onap/policy/drools/system/internal/SimpleLockManagerTest.java
@@ -0,0 +1,781 @@
+/*
+ * ============LICENSE_START=======================================================
+ * ONAP
+ * ================================================================================
+ * Copyright (C) 2019 AT&T Intellectual Property. All rights reserved.
+ * ================================================================================
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ * ============LICENSE_END=========================================================
+ */
+
+package org.onap.policy.drools.system.internal;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anyBoolean;
+import static org.mockito.Matchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Properties;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.Semaphore;
+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.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+import org.onap.policy.common.utils.time.CurrentTime;
+import org.onap.policy.common.utils.time.TestTime;
+import org.onap.policy.drools.core.lock.Lock;
+import org.onap.policy.drools.core.lock.LockCallback;
+import org.onap.policy.drools.core.lock.LockState;
+import org.onap.policy.drools.system.PolicyEngine;
+import org.onap.policy.drools.system.PolicyEngineConstants;
+import org.onap.policy.drools.system.internal.SimpleLockManager.SimpleLock;
+import org.powermock.reflect.Whitebox;
+
+public class SimpleLockManagerTest {
+    private static final String POLICY_ENGINE_EXECUTOR_FIELD = "executorService";
+    private static final String TIME_FIELD = "currentTime";
+    private static final String OWNER_KEY = "my key";
+    private static final String RESOURCE = "my resource";
+    private static final String RESOURCE2 = "my resource #2";
+    private static final String RESOURCE3 = "my resource #3";
+    private static final int HOLD_SEC = 100;
+    private static final int HOLD_SEC2 = 120;
+    private static final int HOLD_MS = HOLD_SEC * 1000;
+    private static final int HOLD_MS2 = HOLD_SEC2 * 1000;
+    private static final int MAX_THREADS = 10;
+    private static final int MAX_LOOPS = 50;
+
+    private static CurrentTime saveTime;
+    private static ScheduledExecutorService saveExec;
+    private static ScheduledExecutorService realExec;
+
+    private TestTime testTime;
+    private AtomicInteger nactive;
+    private AtomicInteger nsuccesses;
+    private SimpleLockManager feature;
+
+    @Mock
+    private ScheduledExecutorService exsvc;
+
+    @Mock
+    private PolicyEngine engine;
+
+    @Mock
+    private ScheduledFuture<?> future;
+
+    @Mock
+    private LockCallback callback;
+
+
+    /**
+     * Saves static fields and configures the location of the property files.
+     */
+    @BeforeClass
+    public static void setUpBeforeClass() {
+        saveTime = Whitebox.getInternalState(SimpleLockManager.class, TIME_FIELD);
+        saveExec = Whitebox.getInternalState(PolicyEngineConstants.getManager(), POLICY_ENGINE_EXECUTOR_FIELD);
+
+        realExec = Executors.newScheduledThreadPool(3);
+        Whitebox.setInternalState(PolicyEngineConstants.getManager(), POLICY_ENGINE_EXECUTOR_FIELD, realExec);
+    }
+
+    /**
+     * Restores static fields.
+     */
+    @AfterClass
+    public static void tearDownAfterClass() {
+        Whitebox.setInternalState(SimpleLockManager.class, TIME_FIELD, saveTime);
+        Whitebox.setInternalState(PolicyEngineConstants.getManager(), POLICY_ENGINE_EXECUTOR_FIELD, saveExec);
+
+        realExec.shutdown();
+    }
+
+    /**
+     * Initializes the mocks and creates a feature that uses {@link #exsvc} to execute
+     * tasks.
+     */
+    @Before
+    public void setUp() {
+        MockitoAnnotations.initMocks(this);
+
+        testTime = new TestTime();
+        nactive = new AtomicInteger(0);
+        nsuccesses = new AtomicInteger(0);
+
+        Whitebox.setInternalState(SimpleLockManager.class, TIME_FIELD, testTime);
+
+        when(engine.getExecutorService()).thenReturn(exsvc);
+
+        feature = new MyLockingFeature();
+        feature.start();
+    }
+
+    /**
+     * Tests constructor() when properties are invalid.
+     */
+    @Test
+    public void testSimpleLockManagerInvalidProperties() {
+        // use properties containing an invalid value
+        Properties props = new Properties();
+        props.setProperty(SimpleLockProperties.EXPIRE_CHECK_SEC, "abc");
+
+        assertThatThrownBy(() -> new MyLockingFeature(engine, props)).isInstanceOf(SimpleLockManagerException.class);
+    }
+
+    @Test
+    public void testIsAlive() {
+        assertTrue(feature.isAlive());
+
+        feature.stop();
+        assertFalse(feature.isAlive());
+    }
+
+    @Test
+    public void testStart() {
+        assertFalse(feature.start());
+
+        feature.stop();
+        assertTrue(feature.start());
+    }
+
+    @Test
+    public void testStop() {
+        assertTrue(feature.stop());
+        verify(future).cancel(true);
+
+        assertFalse(feature.stop());
+
+        // no more invocations
+        verify(future).cancel(anyBoolean());
+    }
+
+    @Test
+    public void testShutdown() {
+        feature.shutdown();
+
+        verify(future).cancel(true);
+    }
+
+    @Test
+    public void testLockApi() {
+        assertFalse(feature.isLocked());
+        assertTrue(feature.lock());
+        assertTrue(feature.unlock());
+    }
+
+    @Test
+    public void testCreateLock() {
+        // this lock should be granted immediately
+        SimpleLock lock = getLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+        assertTrue(lock.isActive());
+        assertEquals(testTime.getMillis() + HOLD_MS, lock.getHoldUntilMs());
+
+        invokeCallback(1);
+
+        verify(callback).lockAvailable(lock);
+        verify(callback, never()).lockUnavailable(lock);
+
+
+        // this time it should be busy
+        Lock lock2 = feature.createLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+        assertFalse(lock2.isActive());
+        assertTrue(lock2.isUnavailable());
+
+        invokeCallback(2);
+
+        verify(callback, never()).lockAvailable(lock2);
+        verify(callback).lockUnavailable(lock2);
+
+        // should have been no change to the original lock
+        assertTrue(lock.isActive());
+        verify(callback).lockAvailable(lock);
+        verify(callback, never()).lockUnavailable(lock);
+
+        // should work with "true" value also
+        Lock lock3 = feature.createLock(RESOURCE2, OWNER_KEY, HOLD_SEC, callback, true);
+        assertTrue(lock3.isActive());
+        invokeCallback(3);
+        verify(callback).lockAvailable(lock3);
+        verify(callback, never()).lockUnavailable(lock3);
+    }
+
+    /**
+     * Tests lock() when the feature is not the latest instance.
+     */
+    @Test
+    public void testCreateLockNotLatestInstance() {
+        SimpleLockManager.setLatestInstance(null);
+
+        Lock lock = feature.createLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+        assertTrue(lock.isUnavailable());
+        verify(callback, never()).lockAvailable(any());
+        verify(callback).lockUnavailable(lock);
+    }
+
+    @Test
+    public void testCheckExpired() throws InterruptedException {
+        final SimpleLock lock = getLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+        final SimpleLock lock2 = getLock(RESOURCE2, OWNER_KEY, HOLD_SEC, callback, false);
+        final SimpleLock lock3 = getLock(RESOURCE3, OWNER_KEY, HOLD_SEC2, callback, false);
+
+        ArgumentCaptor<Runnable> captor = ArgumentCaptor.forClass(Runnable.class);
+        verify(exsvc).scheduleWithFixedDelay(captor.capture(), anyLong(), anyLong(), any());
+
+        Runnable checker = captor.getValue();
+
+        // time unchanged - checker should have no impact
+        checker.run();
+        assertTrue(lock.isActive());
+        assertTrue(lock2.isActive());
+        assertTrue(lock3.isActive());
+
+        // expire the first two locks
+        testTime.sleep(HOLD_MS);
+        checker.run();
+        assertFalse(lock.isActive());
+        assertFalse(lock2.isActive());
+        assertTrue(lock3.isActive());
+
+        // run the callbacks
+        captor = ArgumentCaptor.forClass(Runnable.class);
+        verify(exsvc, times(5)).execute(captor.capture());
+        captor.getAllValues().forEach(Runnable::run);
+        verify(callback).lockUnavailable(lock);
+        verify(callback).lockUnavailable(lock2);
+        verify(callback, never()).lockUnavailable(lock3);
+
+        // should be able to get a lock on the first two resources
+        assertTrue(feature.createLock(RESOURCE, OWNER_KEY, HOLD_SEC + HOLD_SEC2, callback, false).isActive());
+        assertTrue(feature.createLock(RESOURCE2, OWNER_KEY, HOLD_SEC + HOLD_SEC2, callback, false).isActive());
+
+        // lock is still busy on the last resource
+        assertFalse(feature.createLock(RESOURCE3, OWNER_KEY, HOLD_SEC + HOLD_SEC2, callback, false).isActive());
+
+        // expire the last lock
+        testTime.sleep(HOLD_MS2);
+        checker.run();
+        assertFalse(lock3.isActive());
+
+        // run the callback
+        captor = ArgumentCaptor.forClass(Runnable.class);
+        verify(exsvc, times(9)).execute(captor.capture());
+        captor.getValue().run();
+        verify(callback).lockUnavailable(lock3);
+    }
+
+    /**
+     * Tests checkExpired(), where the lock is removed from the map between invoking
+     * expired() and compute(). Should cause "null" to be returned by compute().
+     *
+     * @throws InterruptedException if the test is interrupted
+     */
+    @Test
+    public void testCheckExpiredLockDeleted() throws InterruptedException {
+        feature = new MyLockingFeature() {
+            @Override
+            protected SimpleLock makeLock(LockState waiting, String resourceId, String ownerKey, int holdSec,
+                            LockCallback callback) {
+                return new SimpleLock(waiting, resourceId, ownerKey, holdSec, callback, feature) {
+                    private static final long serialVersionUID = 1L;
+
+                    @Override
+                    public boolean expired(long currentMs) {
+                        // remove the lock from the map
+                        free();
+                        return true;
+                    }
+                };
+            }
+        };
+
+        feature.start();
+
+        feature.createLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+        invokeCallback(1);
+
+        ArgumentCaptor<Runnable> captor = ArgumentCaptor.forClass(Runnable.class);
+        verify(exsvc).scheduleWithFixedDelay(captor.capture(), anyLong(), anyLong(), any());
+
+        Runnable checker = captor.getValue();
+
+        checker.run();
+
+        // lock should now be gone and we should be able to get another
+        feature.createLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+        invokeCallback(2);
+
+        // should have succeeded twice
+        verify(callback, times(2)).lockAvailable(any());
+
+        // lock should not be available now
+        feature.createLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+        invokeCallback(3);
+        verify(callback).lockUnavailable(any());
+    }
+
+    /**
+     * Tests checkExpired(), where the lock is removed from the map and replaced with a
+     * new lock, between invoking expired() and compute(). Should cause the new lock to be
+     * returned.
+     *
+     * @throws InterruptedException if the test is interrupted
+     */
+    @Test
+    public void testCheckExpiredLockReplaced() throws InterruptedException {
+        feature = new MyLockingFeature() {
+            private boolean madeLock = false;
+
+            @Override
+            protected SimpleLock makeLock(LockState waiting, String resourceId, String ownerKey, int holdSec,
+                            LockCallback callback) {
+                if (madeLock) {
+                    return new SimpleLock(waiting, resourceId, ownerKey, holdSec, callback, feature);
+                }
+
+                madeLock = true;
+
+                return new SimpleLock(waiting, resourceId, ownerKey, holdSec, callback, feature) {
+                    private static final long serialVersionUID = 1L;
+
+                    @Override
+                    public boolean expired(long currentMs) {
+                        // remove the lock from the map and add a new lock
+                        free();
+                        feature.createLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+
+                        return true;
+                    }
+                };
+            }
+        };
+
+        feature.start();
+
+        feature.createLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+        invokeCallback(1);
+
+        ArgumentCaptor<Runnable> captor = ArgumentCaptor.forClass(Runnable.class);
+        verify(exsvc).scheduleWithFixedDelay(captor.capture(), anyLong(), anyLong(), any());
+
+        Runnable checker = captor.getValue();
+
+        checker.run();
+
+        // lock should not be available now
+        feature.createLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+        invokeCallback(3);
+        verify(callback).lockUnavailable(any());
+    }
+
+    @Test
+    public void testGetThreadPool() {
+        // use a real feature
+        feature = new SimpleLockManager(engine, new Properties());
+
+        // load properties
+        feature.start();
+
+        // should create thread pool
+        feature.createLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+
+        // should shut down thread pool
+        feature.stop();
+    }
+
+    @Test
+    public void testSimpleLockNoArgs() {
+        SimpleLock lock = new SimpleLock();
+        assertNull(lock.getResourceId());
+        assertNull(lock.getOwnerKey());
+        assertNull(lock.getCallback());
+        assertEquals(0, lock.getHoldSec());
+
+        assertEquals(0, lock.getHoldUntilMs());
+    }
+
+    @Test
+    public void testSimpleLockSimpleLock() {
+        SimpleLock lock = getLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+        assertEquals(RESOURCE, lock.getResourceId());
+        assertEquals(OWNER_KEY, lock.getOwnerKey());
+        assertSame(callback, lock.getCallback());
+        assertEquals(HOLD_SEC, lock.getHoldSec());
+
+        assertThatIllegalArgumentException()
+                        .isThrownBy(() -> feature.createLock(RESOURCE, OWNER_KEY, -1, callback, false))
+                        .withMessageContaining("holdSec");
+    }
+
+    @Test
+    public void testSimpleLockSerializable() throws Exception {
+        SimpleLock lock = getLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+        lock = roundTrip(lock);
+
+        assertTrue(lock.isActive());
+
+        assertEquals(RESOURCE, lock.getResourceId());
+        assertEquals(OWNER_KEY, lock.getOwnerKey());
+        assertNull(lock.getCallback());
+        assertEquals(HOLD_SEC, lock.getHoldSec());
+    }
+
+    @Test
+    public void testSimpleLockExpired() {
+        SimpleLock lock = getLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+        lock.grant();
+
+        assertFalse(lock.expired(testTime.getMillis()));
+        assertFalse(lock.expired(testTime.getMillis() + HOLD_MS - 1));
+        assertTrue(lock.expired(testTime.getMillis() + HOLD_MS));
+    }
+
+    /**
+     * Tests grant() when the lock is already unavailable.
+     */
+    @Test
+    public void testSimpleLockGrantUnavailable() {
+        SimpleLock lock = getLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+        lock.setState(LockState.UNAVAILABLE);
+        lock.grant();
+
+        assertTrue(lock.isUnavailable());
+        verify(callback, never()).lockAvailable(any());
+        verify(callback, never()).lockUnavailable(any());
+    }
+
+    @Test
+    public void testSimpleLockFree() {
+        final SimpleLock lock = getLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+
+        // lock2 should be denied
+        SimpleLock lock2 = getLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+        invokeCallback(2);
+        verify(callback, never()).lockAvailable(lock2);
+        verify(callback).lockUnavailable(lock2);
+
+        // lock2 was denied, so nothing new should happen when freed
+        assertFalse(lock2.free());
+        invokeCallback(2);
+
+        // force lock2 to be active - still nothing should happen
+        Whitebox.setInternalState(lock2, "state", LockState.ACTIVE);
+        assertFalse(lock2.free());
+        invokeCallback(2);
+
+        // now free the first lock
+        assertTrue(lock.free());
+        assertEquals(LockState.UNAVAILABLE, lock.getState());
+
+        // should be able to get the lock now
+        SimpleLock lock3 = getLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+        assertTrue(lock3.isActive());
+    }
+
+    /**
+     * Tests that free() works on a serialized lock with a new feature.
+     *
+     * @throws Exception if an error occurs
+     */
+    @Test
+    public void testSimpleLockFreeSerialized() throws Exception {
+        SimpleLock lock = getLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+
+        feature = new MyLockingFeature();
+        feature.start();
+
+        lock = roundTrip(lock);
+        assertTrue(lock.free());
+        assertTrue(lock.isUnavailable());
+    }
+
+    /**
+     * Tests free() on a serialized lock without a feature.
+     *
+     * @throws Exception if an error occurs
+     */
+    @Test
+    public void testSimpleLockFreeNoFeature() throws Exception {
+        SimpleLock lock = getLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+
+        SimpleLockManager.setLatestInstance(null);
+
+        lock = roundTrip(lock);
+        assertFalse(lock.free());
+        assertTrue(lock.isUnavailable());
+    }
+
+    @Test
+    public void testSimpleLockExtend() {
+        final SimpleLock lock = getLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+
+        // lock2 should be denied
+        SimpleLock lock2 = getLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+        invokeCallback(2);
+        verify(callback, never()).lockAvailable(lock2);
+        verify(callback).lockUnavailable(lock2);
+
+        // lock2 will still be denied
+        lock2.extend(HOLD_SEC, callback);
+        invokeCallback(3);
+        verify(callback, times(2)).lockUnavailable(lock2);
+
+        // force lock2 to be active - should still be denied
+        Whitebox.setInternalState(lock2, "state", LockState.ACTIVE);
+        lock2.extend(HOLD_SEC, callback);
+        invokeCallback(4);
+        verify(callback, times(3)).lockUnavailable(lock2);
+
+        assertThatIllegalArgumentException().isThrownBy(() -> lock.extend(-1, callback))
+                        .withMessageContaining("holdSec");
+
+        // now extend the first lock
+        lock.extend(HOLD_SEC2, callback);
+        assertEquals(HOLD_SEC2, lock.getHoldSec());
+        assertEquals(testTime.getMillis() + HOLD_MS2, lock.getHoldUntilMs());
+        invokeCallback(5);
+        verify(callback).lockAvailable(lock);
+        verify(callback, never()).lockUnavailable(lock);
+    }
+
+    /**
+     * Tests that extend() works on a serialized lock with a new feature.
+     *
+     * @throws Exception if an error occurs
+     */
+    @Test
+    public void testSimpleLockExtendSerialized() throws Exception {
+        SimpleLock lock = getLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+
+        feature = new MyLockingFeature();
+        feature.start();
+
+        lock = roundTrip(lock);
+        LockCallback scallback = mock(LockCallback.class);
+
+        lock.extend(HOLD_SEC, scallback);
+        assertTrue(lock.isActive());
+
+        invokeCallback(1);
+        verify(scallback).lockAvailable(lock);
+        verify(scallback, never()).lockUnavailable(lock);
+    }
+
+    /**
+     * Tests extend() on a serialized lock without a feature.
+     *
+     * @throws Exception if an error occurs
+     */
+    @Test
+    public void testSimpleLockExtendNoFeature() throws Exception {
+        SimpleLock lock = getLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false);
+
+        SimpleLockManager.setLatestInstance(null);
+
+        lock = roundTrip(lock);
+        LockCallback scallback = mock(LockCallback.class);
+
+        lock.extend(HOLD_SEC, scallback);
+        assertTrue(lock.isUnavailable());
+
+        invokeCallback(1);
+        verify(scallback, never()).lockAvailable(lock);
+        verify(scallback).lockUnavailable(lock);
+    }
+
+    @Test
+    public void testSimpleLockToString() {
+        String text = feature.createLock(RESOURCE, OWNER_KEY, HOLD_SEC, callback, false).toString();
+        assertNotNull(text);
+        assertThat(text).contains("holdUntil").doesNotContain("ownerInfo").doesNotContain("callback");
+    }
+
+    /**
+     * Performs a multi-threaded test of the locking facility.
+     *
+     * @throws InterruptedException if the current thread is interrupted while waiting for
+     *         the background threads to complete
+     */
+    @Test
+    public void testMultiThreaded() throws InterruptedException {
+        Whitebox.setInternalState(SimpleLockManager.class, TIME_FIELD, testTime);
+        feature = new SimpleLockManager(PolicyEngineConstants.getManager(), new Properties());
+        feature.start();
+
+        List<MyThread> threads = new ArrayList<>(MAX_THREADS);
+        for (int x = 0; x < MAX_THREADS; ++x) {
+            threads.add(new MyThread());
+        }
+
+        threads.forEach(Thread::start);
+
+        for (MyThread thread : threads) {
+            thread.join(6000);
+            assertFalse(thread.isAlive());
+        }
+
+        for (MyThread thread : threads) {
+            if (thread.err != null) {
+                throw thread.err;
+            }
+        }
+
+        assertTrue(nsuccesses.get() > 0);
+    }
+
+    private SimpleLock getLock(String resource, String ownerKey, int holdSec, LockCallback callback,
+                    boolean waitForLock) {
+        return (SimpleLock) feature.createLock(resource, ownerKey, holdSec, callback, waitForLock);
+    }
+
+    private SimpleLock roundTrip(SimpleLock lock) throws Exception {
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        try (ObjectOutputStream oos = new ObjectOutputStream(baos)) {
+            oos.writeObject(lock);
+        }
+
+        ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray());
+        try (ObjectInputStream ois = new ObjectInputStream(bais)) {
+            return (SimpleLock) ois.readObject();
+        }
+    }
+
+    /**
+     * Invokes the last call-back in the work queue.
+     *
+     * @param nexpected number of call-backs expected in the work queue
+     */
+    private void invokeCallback(int nexpected) {
+        ArgumentCaptor<Runnable> captor = ArgumentCaptor.forClass(Runnable.class);
+        verify(exsvc, times(nexpected)).execute(captor.capture());
+
+        if (nexpected > 0) {
+            captor.getAllValues().get(nexpected - 1).run();
+        }
+    }
+
+    /**
+     * Feature that uses <i>exsvc</i> to execute requests.
+     */
+    private class MyLockingFeature extends SimpleLockManager {
+
+        public MyLockingFeature() {
+            this(engine, new Properties());
+        }
+
+        public MyLockingFeature(PolicyEngine engine, Properties props) {
+            super(engine, props);
+
+            exsvc = mock(ScheduledExecutorService.class);
+            when(engine.getExecutorService()).thenReturn(exsvc);
+
+            when(exsvc.scheduleWithFixedDelay(any(), anyLong(), anyLong(), any())).thenAnswer(answer -> {
+                return future;
+            });
+        }
+    }
+
+    /**
+     * Thread used with the multi-threaded test. It repeatedly attempts to get a lock,
+     * extend it, and then unlock it.
+     */
+    private class MyThread extends Thread {
+        AssertionError err = null;
+
+        public MyThread() {
+            setDaemon(true);
+        }
+
+        @Override
+        public void run() {
+            try {
+                for (int x = 0; x < MAX_LOOPS; ++x) {
+                    makeAttempt();
+                }
+
+            } catch (AssertionError e) {
+                err = e;
+            }
+        }
+
+        private void makeAttempt() {
+            try {
+                Semaphore sem = new Semaphore(0);
+
+                LockCallback cb = new LockCallback() {
+                    @Override
+                    public void lockAvailable(Lock lock) {
+                        sem.release();
+                    }
+
+                    @Override
+                    public void lockUnavailable(Lock lock) {
+                        sem.release();
+                    }
+                };
+
+                Lock lock = feature.createLock(RESOURCE, getName(), HOLD_SEC, cb, false);
+
+                // wait for callback, whether available or unavailable
+                assertTrue(sem.tryAcquire(5, TimeUnit.SECONDS));
+                if (!lock.isActive()) {
+                    return;
+                }
+
+                nsuccesses.incrementAndGet();
+
+                assertEquals(1, nactive.incrementAndGet());
+
+                lock.extend(HOLD_SEC2, cb);
+                assertTrue(sem.tryAcquire(5, TimeUnit.SECONDS));
+                assertTrue(lock.isActive());
+
+                // decrement BEFORE free()
+                nactive.decrementAndGet();
+
+                assertTrue(lock.free());
+                assertTrue(lock.isUnavailable());
+
+            } catch (InterruptedException e) {
+                Thread.currentThread().interrupt();
+                throw new AssertionError("interrupted", e);
+            }
+        }
+    }
+}