diff --git a/fcrepo-http-api/src/main/java/org/fcrepo/http/api/FedoraLocks.java b/fcrepo-http-api/src/main/java/org/fcrepo/http/api/FedoraLocks.java index 2fcb5fb604..bcafb95023 100644 --- a/fcrepo-http-api/src/main/java/org/fcrepo/http/api/FedoraLocks.java +++ b/fcrepo-http-api/src/main/java/org/fcrepo/http/api/FedoraLocks.java @@ -96,23 +96,18 @@ public RdfStream getLock(@PathParam("path") final List 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 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()); diff --git a/fcrepo-http-api/src/test/java/org/fcrepo/http/api/FedoraLocksTest.java b/fcrepo-http-api/src/test/java/org/fcrepo/http/api/FedoraLocksTest.java index bfb1cb8e3b..6fb9940da2 100644 --- a/fcrepo-http-api/src/test/java/org/fcrepo/http/api/FedoraLocksTest.java +++ b/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()); } diff --git a/fcrepo-http-api/src/test/java/org/fcrepo/integration/http/api/FedoraLocksIT.java b/fcrepo-http-api/src/test/java/org/fcrepo/integration/http/api/FedoraLocksIT.java index 7b0c5dc5f0..20e3ff7a01 100644 --- a/fcrepo-http-api/src/test/java/org/fcrepo/integration/http/api/FedoraLocksIT.java +++ b/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); } diff --git a/fcrepo-kernel-api/src/main/java/org/fcrepo/kernel/Lock.java b/fcrepo-kernel-api/src/main/java/org/fcrepo/kernel/Lock.java index 70a548fde4..d738095cc9 100644 --- a/fcrepo-kernel-api/src/main/java/org/fcrepo/kernel/Lock.java +++ b/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(); - } diff --git a/fcrepo-kernel-api/src/main/java/org/fcrepo/kernel/services/LockService.java b/fcrepo-kernel-api/src/main/java/org/fcrepo/kernel/services/LockService.java index 2a261cc031..847014cffa 100644 --- a/fcrepo-kernel-api/src/main/java/org/fcrepo/kernel/services/LockService.java +++ b/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 diff --git a/fcrepo-kernel/src/main/java/org/fcrepo/kernel/services/LockServiceImpl.java b/fcrepo-kernel/src/main/java/org/fcrepo/kernel/services/LockServiceImpl.java index 287d808738..ff9b96b75d 100644 --- a/fcrepo-kernel/src/main/java/org/fcrepo/kernel/services/LockServiceImpl.java +++ b/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(); } diff --git a/fcrepo-kernel/src/test/java/org/fcrepo/kernel/services/LockServiceImplTest.java b/fcrepo-kernel/src/test/java/org/fcrepo/kernel/services/LockServiceImplTest.java index 754604bc63..66ea5ddc92 100644 --- a/fcrepo-kernel/src/test/java/org/fcrepo/kernel/services/LockServiceImplTest.java +++ b/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); } }