From 4d3189261b2fedbcc2a2327aa7a3ac1efc91b135 Mon Sep 17 00:00:00 2001 From: Moshe Good Date: Tue, 13 Feb 2024 11:44:35 -0500 Subject: [PATCH 1/3] ShouldSkipBlockingWait should still acquire a dead lock if tried for longer than TTL --- .../dynamodbv2/AmazonDynamoDBLockClient.java | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java b/src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java index da30ee4..cd5170e 100644 --- a/src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java +++ b/src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java @@ -233,6 +233,7 @@ public class AmazonDynamoDBLockClient implements Runnable, Closeable { private final boolean holdLockOnServiceUnavailable; private final String ownerName; private final ConcurrentHashMap locks; + private final ConcurrentHashMap notMyLocks; private final ConcurrentHashMap sessionMonitors; private final Optional backgroundThread; private final Function namedThreadCreator; @@ -470,11 +471,26 @@ public LockItem acquireLock(final AcquireLockOptions options) throws LockNotGran } if (options.shouldSkipBlockingWait() && existingLock.isPresent() && !existingLock.get().isExpired()) { + String id = existingLock.get().getUniqueIdentifier(); + // Let's check to see if this existingLock expired based on old data we cached. + // Or cache it if we haven't seen this recordVersion before. + boolean itReallyIsExpired = false; + if (notMyLocks.containsKey(id) && + notMyLocks.get(id).getRecordVersionNumber() + .equals(existingLock.get().getRecordVersionNumber())) { + itReallyIsExpired = notMyLocks.get(id).isExpired(); + } else { + notMyLocks.put(id, existingLock.get()); + } + /* * The lock is being held by some one and is still not expired. And the caller explicitly said not to perform a blocking wait; * We will throw back a lock not grant exception, so that the caller can retry if needed. */ - throw new LockCurrentlyUnavailableException("The lock being requested is being held by another client."); + + if (!itReallyIsExpired) { + throw new LockCurrentlyUnavailableException("The lock being requested is being held by another client."); + } } Optional newLockData = Optional.empty(); From dbc76cc5cf63c4987797f5f340a198cb61ff7678 Mon Sep 17 00:00:00 2001 From: Moshe Good Date: Thu, 15 Feb 2024 13:14:40 -0500 Subject: [PATCH 2/3] Do not wait once we know a lease is expired --- .../dynamodbv2/AmazonDynamoDBLockClient.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java b/src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java index cd5170e..8174459 100644 --- a/src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java +++ b/src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java @@ -474,11 +474,16 @@ public LockItem acquireLock(final AcquireLockOptions options) throws LockNotGran String id = existingLock.get().getUniqueIdentifier(); // Let's check to see if this existingLock expired based on old data we cached. // Or cache it if we haven't seen this recordVersion before. - boolean itReallyIsExpired = false; + boolean isReallyExpired = false; if (notMyLocks.containsKey(id) && notMyLocks.get(id).getRecordVersionNumber() .equals(existingLock.get().getRecordVersionNumber())) { - itReallyIsExpired = notMyLocks.get(id).isExpired(); + + isReallyExpired = notMyLocks.get(id).isExpired(); + if (isReallyExpired) { + // short circuit the waiting that we normally do. + lockTryingToBeAcquired = notMyLocks.get(id); + } } else { notMyLocks.put(id, existingLock.get()); } @@ -487,8 +492,7 @@ public LockItem acquireLock(final AcquireLockOptions options) throws LockNotGran * The lock is being held by some one and is still not expired. And the caller explicitly said not to perform a blocking wait; * We will throw back a lock not grant exception, so that the caller can retry if needed. */ - - if (!itReallyIsExpired) { + if (!isReallyExpired) { throw new LockCurrentlyUnavailableException("The lock being requested is being held by another client."); } } From f620e0a2ba06c73cf408235af93b4c0d442f4c26 Mon Sep 17 00:00:00 2001 From: Moshe Good Date: Thu, 15 Feb 2024 13:37:13 -0500 Subject: [PATCH 3/3] Add tests for eventually getting the lock when SkipBlockingWait is set --- .../dynamodbv2/AmazonDynamoDBLockClient.java | 2 +- .../AmazonDynamoDBLockClientTest.java | 39 ++++++++++++++++++- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java b/src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java index 8174459..4d7d0f8 100644 --- a/src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java +++ b/src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java @@ -233,7 +233,7 @@ public class AmazonDynamoDBLockClient implements Runnable, Closeable { private final boolean holdLockOnServiceUnavailable; private final String ownerName; private final ConcurrentHashMap locks; - private final ConcurrentHashMap notMyLocks; + private final ConcurrentHashMap notMyLocks = new ConcurrentHashMap<>(); private final ConcurrentHashMap sessionMonitors; private final Optional backgroundThread; private final Function namedThreadCreator; diff --git a/src/test/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClientTest.java b/src/test/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClientTest.java index 0480d56..6c11a56 100644 --- a/src/test/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClientTest.java +++ b/src/test/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClientTest.java @@ -443,10 +443,45 @@ public void acquireLock_whenLockAlreadyExistsAndIsNotReleased_andSkipBlockingWai .thenReturn(GetItemResponse.builder().item(item).build()) .thenReturn(GetItemResponse.builder().build()); AcquireLockOptions acquireLockOptions = AcquireLockOptions.builder("customer1") - .withShouldSkipBlockingWait(true) - .withDeleteLockOnRelease(false).build(); + .withShouldSkipBlockingWait(true) + .withDeleteLockOnRelease(false).build(); client.acquireLock(acquireLockOptions); } + /* + * Test case for the scenario, where the lock is being held by the first owner and the lock duration has not past + * the lease duration. In this case, We should expect a LockAlreadyOwnedException when shouldSkipBlockingWait is set. + * But if we try again later, we should get the lock. + */ + @Test + public void acquireLock_whenLockAlreadyExistsAndIsNotReleased_andSkipBlockingWait_eventuallyGetsTheLock() + throws InterruptedException { + UUID uuid = setOwnerNameToUuid(); + AmazonDynamoDBLockClient client = getLockClient(); + Map item = new HashMap<>(5); + item.put("customer", AttributeValue.builder().s("customer1").build()); + item.put("ownerName", AttributeValue.builder().s("foobar").build()); + item.put("recordVersionNumber", AttributeValue.builder().s(uuid.toString()).build()); + item.put("leaseDuration", AttributeValue.builder().s("100").build()); + when(dynamodb.getItem(Mockito.any())) + .thenReturn(GetItemResponse.builder().item(item).build()) + .thenReturn(GetItemResponse.builder().build()); + AcquireLockOptions acquireLockOptions = AcquireLockOptions.builder("customer1") + .withShouldSkipBlockingWait(true) + .withDeleteLockOnRelease(false).build(); + + try { + client.acquireLock(acquireLockOptions); + } catch (LockCurrentlyUnavailableException e) { + // This is expected + } catch (RuntimeException e) { + Assert.fail("Expected LockCurrentlyUnavailableException, but got " + e.getClass().getName()); + } + + // Now wait for the TTL to expire and try to acquire the lock again + Thread.sleep(101); + LockItem lockItem = client.acquireLock(acquireLockOptions); + Assert.assertNotNull("Failed to get lock item, when the lock is not present in the db", lockItem); + } @Test(expected = IllegalArgumentException.class) public void sendHeartbeat_whenDeleteDataTrueAndDataNotNull_throwsIllegalArgumentException() {