Remove concept of lock timeout.

- Remove requirement for a Lock-Ticket to delete a lock

Resolves: https://www.pivotaltracker.com/story/show/69737940

fcrepo-http-api/src/main/java/org/fcrepo/http/api/FedoraLocks.java

@@ -96,23 +96,18 @@ public RdfStream getLock(@PathParam("path") final List<PathSegment> pathList) th
      * Creates a lock at the given path that is tied to the current session.
      * Only the current session may make changes to the current path while
      * the lock is held.
-     * @param timeout a number of seconds before which the lock will not be removed
-     *                by the system.  (note, this is just a hint at the expected
-     *                lifespan of the lock, the system makes no guarantees that it
-     *                will actually be removed)
      * @param isDeep if true the created lock will affect all nodes in the subgraph
      *               below
      */
     @POST
     @Timed
     public Response createLock(@PathParam("path") final List<PathSegment> pathList,
-                               @QueryParam("timeout") @DefaultValue("-1") final long timeout,
                                @QueryParam("deep") @DefaultValue("false") final boolean isDeep)
         throws RepositoryException, URISyntaxException {
         try {
             final String path = toPath(pathList);
             final Node node = session.getNode(path);
-            final Lock lock = lockService.acquireLock(session, path, timeout, isDeep);
+            final Lock lock = lockService.acquireLock(session, path, isDeep);
             session.save();
             final String location = getTranslator().getSubject(node.getPath()).getURI();
             LOGGER.debug("Locked {} with lock token {}.", path, lock.getLockToken());

fcrepo-http-api/src/test/java/org/fcrepo/http/api/FedoraLocksTest.java

@@ -49,8 +49,6 @@
  */
 public class FedoraLocksTest {
 
-    private static final long timeout = 300;
-
     FedoraLocks testObj;
 
     @Mock
@@ -97,11 +95,11 @@ public void testCreateLock() throws RepositoryException, URISyntaxException {
         final String pid = UUID.randomUUID().toString();
         final String path = "/" + pid;
         initializeMockNode(path);
-        when(mockLockService.acquireLock(mockSession, path, timeout, false)).thenReturn(mockLock);
+        when(mockLockService.acquireLock(mockSession, path, false)).thenReturn(mockLock);
 
-        final Response response = testObj.createLock(createPathList(pid), timeout, false);
+        final Response response = testObj.createLock(createPathList(pid), false);
 
-        verify(mockLockService).acquireLock(mockSession, path, timeout, false);
+        verify(mockLockService).acquireLock(mockSession, path, false);
         Assert.assertEquals(Response.Status.CREATED.getStatusCode(), response.getStatus());
     }
 

fcrepo-http-api/src/test/java/org/fcrepo/integration/http/api/FedoraLocksIT.java

@@ -45,7 +45,6 @@
 import static javax.ws.rs.core.Response.Status.NO_CONTENT;
 import static org.fcrepo.kernel.RdfLexicon.HAS_LOCK_TOKEN;
 import static org.fcrepo.kernel.RdfLexicon.LOCKS;
-import static org.junit.Assert.assertEquals;
 import static org.slf4j.LoggerFactory.getLogger;
 
 /**
@@ -55,8 +54,6 @@ public class FedoraLocksIT extends AbstractResourceIT implements FedoraJcrTypes
 
     private static final Logger LOGGER = getLogger(FedoraLocksIT.class);
 
-    private static final long TIMEOUT = 300;
-
     /**
      * Test whether a lock can be created, it prevents updates
      * without the token, allows updates with the token and can
@@ -74,13 +71,18 @@ public void testBasicLockingScenario() throws IOException {
 
         assertCanSetProperty("With the lock token, property updates must be allowed!", pid, lockToken);
 
-        Assert.assertEquals("The lock must only be able to be removed with the lock token!",
-                CONFLICT.getStatusCode(),
-                unlockObject(pid, null).getStatusLine().getStatusCode());
-
         assertUnlockWithToken(pid, lockToken);
     }
 
+    @Test
+    public void testUnlockWithoutToken() throws IOException {
+        final String pid = getRandomUniquePid();
+        createObject(pid);
+
+        getLockToken(lockObject(pid));
+        assertUnlockWithoutToken(pid);
+    }
+
     /**
      * Test whether a created lock can be viewed by both
      * the creator (supplying the token) and by a request
@@ -137,7 +139,7 @@ public void testDeepVsShallowLocks() throws IOException {
         assertUnlockWithToken(pid, shallowLockToken);
 
         // Test deep lock
-        final String deepLockToken = getLockToken(lockObject(pid, TIMEOUT, true));
+        final String deepLockToken = getLockToken(lockObject(pid, true));
 
         assertCannotSetPropertyWithoutLockToken(pid);
         assertCannotSetPropertyWithoutLockToken("Deep lock must prevent property updates on child nodes!", childPid);
@@ -192,7 +194,7 @@ public void testConflictingDeepLocks() throws IOException {
 
         final String childLockToken = getLockToken(lockObject(childPid));
         Assert.assertEquals("May not take out a deep lock when a child is locked!",
-                CONFLICT.getStatusCode(), lockObject(pid, TIMEOUT, true).getStatusLine().getStatusCode());
+                CONFLICT.getStatusCode(), lockObject(pid, true).getStatusLine().getStatusCode());
     }
 
     /**
@@ -262,7 +264,7 @@ public void testTransactionFailureOnCommit() throws IOException {
                 txId + "/" + childPid, null);
 
         // take out a lock on an affected resource (out of transaction)
-        final String lockToken = getLockToken(lockObject(rootPid, TIMEOUT, true));
+        final String lockToken = getLockToken(lockObject(rootPid, true));
 
         // commit the transaction (which should fail with CONFLICT)
         Assert.assertEquals(CONFLICT.getStatusCode(), commitTransaction(txId).getStatusLine().getStatusCode());
@@ -272,65 +274,14 @@ public void testTransactionFailureOnCommit() throws IOException {
     }
 
     /**
-     * Test whether a created and abandoned lock will actually timeout
-     * within the ballpark of the specified timeout interval.
-     *
-     * Modeshape's lock cleanup interval is not configurable and is
-     * currently set to around 10 minutes.  It is not practical to verify
-     * this during our integration tests, so this test is ignored.
-     *
-     * Furthermore, this test fails anyway.
+     * Unlocks a lock with no token, asserts that it is successful and further
+     * asserts that property updates can again be made without the token.
      */
-    @Test
-    @Ignore
-    public void testLockTimeout() throws IOException {
-        final long maxWaitTimeMs = 6000000;
-        final long timeoutInSeconds = 5;
-        final long timeoutInMs = timeoutInSeconds * 1000;
-        final String pid = getRandomUniquePid();
-        createObject(pid);
-        LOGGER.info("Starting lock timeout test -- May take up to " + maxWaitTimeMs + "ms");
-        final long start = System.currentTimeMillis();
-        getLockToken(lockObject(pid, timeoutInSeconds, false));
-        final long timeout = start + timeoutInMs;
-        final int statusOfImmediateRequest
-                = setProperty(pid, null, null, "test", "test").getStatusLine().getStatusCode();
-        final long timeOfImmediateRequestCompletion = System.currentTimeMillis();
-        final long msUntilImmediateRequest = timeOfImmediateRequestCompletion - start;
-        if (msUntilImmediateRequest >= (timeoutInMs)) {
-            Assert.fail("Tests are running too slow to gauge timeouts! (2 requests took "
-                    + msUntilImmediateRequest + "ms!");
-        } else {
-            Assert.assertEquals("Request within " + msUntilImmediateRequest
-                    + "ms of lock creation should be affected by a " + timeoutInMs
-                    + "ms lock!", CONFLICT.getStatusCode(), statusOfImmediateRequest);
-        }
-        long sleepTime = 1000;
-        while (System.currentTimeMillis() < (start + maxWaitTimeMs)) {
-            try {
-                Thread.sleep(sleepTime);
-            } catch (InterruptedException e) {
-                // no worries.. we'll sleep again when the loop iterates
-            }
-            sleepTime *=2;
-            final long attemptTime = System.currentTimeMillis();
-            final int status = setProperty(pid, null, null, "test", "test").getStatusLine().getStatusCode();
-            final long duration = attemptTime - start;
-            if (status != NO_CONTENT.getStatusCode()) {
-                if (duration > timeoutInMs) {
-                    LOGGER.warn("After " + duration + "ms, the lock still holds.");
-                } else {
-                    LOGGER.info("After " + duration + "ms, the lock still holds.");
-                }
-            } else {
-                Assert.assertTrue("Lock must be held as long as the specified timeout! (unlocked after "
-                        + duration + "ms).", duration > timeoutInMs);
-                return;
-            }
-        }
-        Assert.assertEquals("The " + timeoutInMs + "ms lock should have timed out after "
-                + (System.currentTimeMillis() - start) + "ms!", NO_CONTENT.getStatusCode(),
-                setProperty(pid, null, null, "test", "test").getStatusLine().getStatusCode());
+    private void assertUnlockWithoutToken(String pid) throws IOException {
+        Assert.assertEquals(NO_CONTENT.getStatusCode(),
+                unlockObject(pid, null).getStatusLine().getStatusCode());
+
+        assertCanSetProperty("Unlocked object must be able to be updated now!", pid, null);
     }
 
     /**
@@ -361,26 +312,15 @@ private String getLockToken(HttpResponse response) {
      * Attempts to lock an object.
      */
     private HttpResponse lockObject(String pid) throws IOException {
-        return lockObject(pid, TIMEOUT, false);
+        return lockObject(pid, false);
     }
 
     /**
-     * Attempts to lock an object with the given timeout and
-     * deep locking status.
+     * Attempts to lock an object with the given deep locking status.
      */
-    private HttpResponse lockObject(String pid, long timeout, boolean deep) throws IOException {
-        StringBuffer query = new StringBuffer();
-        if (timeout >= 1) {
-            query.append("timeout=" + timeout);
-        }
-        if (deep) {
-            if (query.length() > 0) {
-                query.append("&");
-            }
-            query.append("deep=true");
-        }
+    private HttpResponse lockObject(String pid, boolean deep) throws IOException {
         final HttpPost post = new HttpPost(serverAddress + pid + "/" + FCR_LOCK
-                + (query.length() > 0 ? "?" + query.toString() : ""));
+                + (deep  ? "?deep=true" : "?deep=false"));
         return client.execute(post);
     }
 

fcrepo-kernel-api/src/main/java/org/fcrepo/kernel/Lock.java

@@ -33,5 +33,4 @@ public interface Lock {
      * entire subgraph under the locked node.
      */
     public boolean isDeep();
-
 }

fcrepo-kernel-api/src/main/java/org/fcrepo/kernel/services/LockService.java

@@ -30,14 +30,12 @@ public interface LockService extends Service {
      * Acquires a lock on the node at the given path.
      * @param session the session wishing to lock the node/subgraph
      * @param path the path to be affected
-     * @param timeout the number of seconds before which the system
-     *                may not remove the lock.
      * @param deep if true, indicates that the entire subgraph starting
      *             at the given path is to be locked
      * @return a Lock object that contains a lock token that will allow
      *         other sessions to access resources locked by this call.
      */
-    Lock acquireLock(Session session, String path, long timeout, boolean deep) throws RepositoryException;
+    Lock acquireLock(Session session, String path, boolean deep) throws RepositoryException;
 
     /**
      * Gets the lock at the given path.  The returned lock will only

fcrepo-kernel/src/main/java/org/fcrepo/kernel/services/LockServiceImpl.java

@@ -22,7 +22,6 @@
 import javax.jcr.PathNotFoundException;
 import javax.jcr.RepositoryException;
 import javax.jcr.Session;
-import javax.jcr.lock.LockException;
 import javax.jcr.lock.LockManager;
 
 /**
@@ -32,11 +31,10 @@
 public class LockServiceImpl extends AbstractService implements LockService  {
 
     @Override
-    public Lock acquireLock(final Session session, final String path, long timeout, boolean deep)
+    public Lock acquireLock(final Session session, final String path, final boolean deep)
         throws RepositoryException {
         final LockManager lockManager = session.getWorkspace().getLockManager();
-        final Lock lock = new JCRLock(lockManager.lock(path, deep, false, timeout, session.getUserID()));
-        return lock;
+        return  new JCRLock(lockManager.lock(path, deep, false, -1, session.getUserID()));
     }
 
     @Override
@@ -54,9 +52,6 @@ public void releaseLock(final Session session, final String path) throws Reposit
         if (!lockManager.isLocked(path)) {
             throw new PathNotFoundException("No lock at path " + path + "!");
         }
-        if (!lockManager.getLock(path).isLockOwningSession()) {
-            throw new LockException("Lock is not held by this session!");
-        }
         session.getWorkspace().getLockManager().unlock(path);
     }
 
@@ -66,7 +61,7 @@ private static class JCRLock implements Lock {
 
         private String token;
 
-        public JCRLock(final javax.jcr.lock.Lock lock) {
+        public JCRLock(final javax.jcr.lock.Lock lock) throws RepositoryException {
             isDeep = lock.isDeep();
             token = lock.getLockToken();
         }

fcrepo-kernel/src/test/java/org/fcrepo/kernel/services/LockServiceImplTest.java

@@ -41,7 +41,7 @@ public class LockServiceImplTest {
 
     private static final String ALREADY_LOCKED_PATH= "test2";
 
-    private static final long TIMEOUT = 300;
+    private static final long TIMEOUT = -1;
 
     private static final String USER = "user";
 
@@ -77,23 +77,21 @@ public void setUp() throws RepositoryException {
         when(mockLockManager.lock(LOCKABLE_PATH, false, false, TIMEOUT, USER)).thenReturn(mockLock);
         when(mockLockManager.lock(ALREADY_LOCKED_PATH, false, false, TIMEOUT, USER)).thenThrow(LockException.class);
         when(mockLockManager.isLocked(ALREADY_LOCKED_PATH)).thenReturn(true);
-        when(mockLock.isLockOwningSession()).thenReturn(true);
         when(mockLock.getLockToken()).thenReturn(LOCK_TOKEN);
         when(mockLock.isDeep()).thenReturn(false);
-        when(otherMockLock.isLockOwningSession()).thenReturn(false);
         when(otherMockLock.isDeep()).thenReturn(false);
     }
 
     @Test
     public void testAcquireLock() throws RepositoryException {
-        final org.fcrepo.kernel.Lock lock = testObj.acquireLock(mockSession, LOCKABLE_PATH, TIMEOUT, false);
+        final org.fcrepo.kernel.Lock lock = testObj.acquireLock(mockSession, LOCKABLE_PATH, false);
         Assert.assertEquals(LOCK_TOKEN, lock.getLockToken());
         Assert.assertFalse(lock.isDeep());
     }
 
     @Test (expected = LockException.class)
     public void testAcquireLockFailure() throws RepositoryException {
-        final org.fcrepo.kernel.Lock lock = testObj.acquireLock(mockSession, ALREADY_LOCKED_PATH, TIMEOUT, false);
+        final org.fcrepo.kernel.Lock lock = testObj.acquireLock(mockSession, ALREADY_LOCKED_PATH, false);
     }
 
     @Test
@@ -111,9 +109,10 @@ public void testReleaseOwnedLock() throws RepositoryException {
         verify(mockLockManager).unlock(LOCKABLE_PATH);
     }
 
-    @Test (expected = LockException.class)
+    @Test
     public void testReleaseOtherLock() throws RepositoryException {
         testObj.releaseLock(mockSession, ALREADY_LOCKED_PATH);
+        verify(mockLockManager).unlock(ALREADY_LOCKED_PATH);
     }
 
 }