Add a withClockSkewUpperBound option when acquiring a lock #88

tso · 2023-03-10T17:42:10Z

This commit addresses issue #44 by providing a new option to utilize wall clock time along with a provided error bound to determine if a lock is expired.

Previously it was impossible to determine if a lease was expired without blocking for at least lease duration and seeing if the version had changed. Thus, if you never block and the lease wasn't explicitly marked as released then the lock was unable to ever be acquired again. By providing a correct upper clock skew error bound clients can correctly take over locks which have expired but not been explicitly released without blocking.

In most distributed systems relying on wall clock time is generally not correct but in this case we can provide an upper clock skew error bound on the scale of minutes without facing any negative consequences for most clients. This tradeoff of having a lease be unacquireable for several minutes is possibly better than being forced to block in many use cases.

This commit addresses issue awslabs#44 by providing a new option to utilize wall clock time along with a provided error bound to determine if a lock is expired. Previously it was impossible to determine if a lease was expired without blocking for at least lease duration and seeing if the version had changed. Thus, if you never block and the lease wasn't explicitly marked as released then the lock was unable to ever be acquired again. By providing a correct upper clock skew error bound clients can correctly take over locks which have expired but not been explicitly released without blocking. In most distributed systems relying on wall clock time is generally not correct but in this case we can provide an upper clock skew error bound on the scale of minutes without facing any negative consequences for most clients. This tradeoff of having a lease be unacquireable for several minutes is possibly better than being forced to block in many use cases.

qvissak · 2023-06-19T19:32:52Z

src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java

            throw new IllegalArgumentException(String
-                .format("Additional attribute cannot be one of the following types: " + "%s, %s, %s, %s, %s", this.partitionKeyName, OWNER_NAME, LEASE_DURATION,
-                    RECORD_VERSION_NUMBER, DATA));
+                .format("Additional attribute cannot be one of the following types: " + "%s, %s, %s, %s, %s %s", this.partitionKeyName, OWNER_NAME, LEASE_DURATION,


missing comma

qvissak · 2023-06-19T19:33:08Z

src/main/java/com/amazonaws/services/dynamodbv2/AcquireLockOptions.java

@@ -90,6 +92,7 @@ public static class AcquireLockOptionsBuilder {
            this.shouldSkipBlockingWait = false;
            this.acquireReleasedLocksConsistently = false;
            this.reentrant = false;
+            this.clockSkewUpperBound = Optional.empty();;


qvissak · 2023-06-19T19:51:39Z

src/main/java/com/amazonaws/services/dynamodbv2/LockItem.java

@@ -298,6 +313,10 @@ void updateRecordVersionNumber(final String recordVersionNumber, final long last
        this.leaseDuration.set(leaseDurationToEnsureInMilliseconds);
    }

+    void updateLastTouchedAt(long lastTouchedAt) {


missing docstring

qvissak · 2023-06-20T18:47:10Z

src/test/java/com/amazonaws/services/dynamodbv2/BasicLockClientTests.java

+            .withCreateHeartbeatBackgroundThread(false)
+            .build());
+
+        // Acquire lock successfully. Note the lack of heartbeats.


Comment needs to be updated

qvissak · 2023-06-23T16:17:50Z

src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java


            //Set up update expression for UpdateItem.
-            final String updateExpression;
+            StringBuilder updateExpression = new StringBuilder();


Should pass 32 here to the constructor since the default is 16 and at minimum the expression will be 29 char long

qvissak · 2023-06-23T16:18:49Z

src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java

@@ -497,14 +497,47 @@ public LockItem acquireLock(final AcquireLockOptions options) throws LockNotGran
                    item.put(RECORD_VERSION_NUMBER, AttributeValue.builder().s(String.valueOf(recordVersionNumber)).build());
                    sortKeyName.ifPresent(sortKeyName -> item.put(sortKeyName, AttributeValue.builder().s(sortKey.get()).build()));
                    newLockData.ifPresent(byteBuffer -> item.put(DATA, AttributeValue.builder().b(SdkBytes.fromByteBuffer(byteBuffer)).build()));
+                    Optional<AtomicLong> lastTouchedAt = options.getClockSkewUpperBound()
+                      .map(unused -> new AtomicLong(System.currentTimeMillis()));


Would be more readable if you did:

Optional<AtomicLong> lastTouchedAt = Optional.empty(); if (options.getClockSkewUpperBound().isPresent()) { lastTouchedAt = Optional.of(new AtomicLong(System.currentTimeMillis())); }

qvissak · 2023-06-23T16:19:34Z

src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java

+                            if (lockItem.getLastTouchedAt().isPresent()) {
+                                long currentTime = System.currentTimeMillis();
+                                // If the lock hasn't been touched since the lease duration plus error bound then it's ours!
+                                if (currentTime > lockItem.getLastTouchedAt().get().get() + lockItem.getLeaseDuration() + clockSkewUpperBound) {


It would be more readable if this logic was moved into LockItem, similar to the isExpired method. You could do something like isExpired(clockSkewUpperBound).

Implementation:

/** * Returns whether or not the lock is expired, based on if the lock hasn't been touched since the lease duration plus cloud skew error threshold * * @param clockSkewUpperBound the amount of time to add to the lease duration to account for clock skew precision errors * @return True if the lock is expired, false otherwise */ public boolean isExpired(Long clockSkewUpperBound) { if (this.isReleased) { return true; } if (this.lastTouchedAt.isPresent()) { long currentTime = System.currentTimeMillis(); // If the lock hasn't been touched since the lease duration plus error bound then it's ours! return currentTime > this.lastTouchedAt.get().get() + this.leaseDuration.get() + clockSkewUpperBound; } return false; }

qvissak · 2023-06-26T21:43:48Z

src/main/java/com/amazonaws/services/dynamodbv2/AcquireLockOptions.java

+         * It's critically important that this error bound is accurate to the nodes that are relying on the lock client. If not,
+         * correctness problems can occur.
+         *
+         * @param clockSkewUpperBound the upper error bound of clock skew across the nodes running this client


I would make this clear that this value is in milliseconds.

qvissak · 2023-06-29T18:13:06Z

src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java

+                      .map(unused -> new AtomicLong(System.currentTimeMillis()));
+                    lastTouchedAt.ifPresent(value -> item.put(LAST_TOUCHED_AT, AttributeValue.builder().s(String.valueOf(value.get())).build()));
+
+                    if (options.shouldSkipBlockingWait() && existingLock.isPresent() && !existingLock.get().isExpired()) {


This won't work and introduces a bug for backwards compatibility/those customers not using the new option. existingLock.get().isExpired() will always be false and this if statement will evaluate to true for people NOT using clockSkewUpperBound. Therefore, LockCurrentlyUnavailableException will be thrown whether the lock is expired or not.

existingLock.get().isExpired() will always be false because it's using the current time minus the time that we just READ the lock from DDB. So when you subtract these two times, you'll get a time between ms and ns and compare to see if that is greater than the leaseDuration, which is almost certainly is not.

Effectively this prevents people from acquiring a lock that existed before the deployment of this update.

qvissak

This introduces a bug for those not using the clockSkew option.

qvissak · 2023-06-29T20:56:11Z

src/main/java/com/amazonaws/services/dynamodbv2/AmazonDynamoDBLockClient.java

+                         * Note that if we don't provide an upper clock skew error bound then we will NEED to wait to acquire a lock which is
+                         * expired but failed to be released/deleted due to ungraceful shutdown of the owning node.
+                         */
+                        throw new LockCurrentlyUnavailableException("The lock being requested is being held by another client.");


To fix the bug I described above, you need to wrap this throw statement with a conditional:

if (!hasExpiredRecordRevisionNumber) { throw new LockCurrentlyUnavailableException("The lock being requested is being held by another client."); }

And of course update the code comment.

And add a unit test.

Oh I see, that makes sense I think!

Just as an fyi I've been busy with some stuff recently and haven't had time to address your comments but I really appreciate them. Will spend some time next week on this!

I'm working on getting approval from Amazon to publish open source code. Once I do that, I can add the unit test in another CR so that you can confidently fix your PR. The unit test has been missing for years.

I've already added it internally, added that additional 'if' statement to your code and merged your changes and tested them myself, and all looks good after that.

Is

if (!hasExpiredRecordRevisionNumber) { throw new LockCurrentlyUnavailableException("The lock being requested is being held by another client."); }

equivalent to

if (!lockTryingToBeAcquired.getRecordVersionNumber().equals(existingLock.get().getRecordVersionNumber())) { throw new LockCurrentlyUnavailableException("The lock being requested is being held by another client."); }

? I wasn't sure what determines a record number being expired -- I think it would be when the local is different than the remote?

Being honest, I thought that

This won't work and introduces a bug for backwards compatibility/those customers not using the new option. existingLock.get().isExpired() will always be false and this if statement will evaluate to true for people NOT using clockSkewUpperBound. Therefore, LockCurrentlyUnavailableException will be thrown whether the lock is expired or not.

was the case currently? e.g. if you don't blocking wait then you'll never be able to acquire a lock if it exists in dynamo

I'm working on getting approval from Amazon to publish open source code. Once I do that, I can add the unit test in another CR so that you can confidently fix your PR. The unit test has been missing for years.

I'll keep an eye out for this and rebase on top once merged!

rahulvishwanatham · 2023-11-01T17:50:27Z

Any update on this merge please? we are using this library and are stuck with this issue of instances not able to acquire locks when DDB has stale locks, Please reply so that we can wait for the new version or choose a different approach.

Thanks in Advance

nathanallen-toast · 2023-12-27T16:14:12Z

Bumping the above question, we'd also love to see this merged!

lcabancla · 2024-02-16T15:54:50Z

FYI, there is an alternative fix that doesn't require using a wall clock time.

tso mentioned this pull request Jun 14, 2023

Fix: Lock can be acquired after lease duration if the current owner has been terminated unexpectedly when using skipBlockingWait=true #91

Closed

qvissak reviewed Jun 19, 2023

View reviewed changes

qvissak reviewed Jun 20, 2023

View reviewed changes

qvissak suggested changes Jun 23, 2023

View reviewed changes

qvissak reviewed Jun 26, 2023

View reviewed changes

qvissak reviewed Jun 29, 2023

View reviewed changes

qvissak suggested changes Jun 29, 2023

View reviewed changes

qvissak reviewed Jun 29, 2023

View reviewed changes

qvissak mentioned this pull request Jun 29, 2023

Fix: Lock can be acquired after lease duration if the current owner has been terminated unexpectedly when using skipBlockingWait=true #79

Closed

Address style/comment comments

ef87379

ethkatnic mentioned this pull request Oct 9, 2024

Fix test coverage and update maven version #104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a withClockSkewUpperBound option when acquiring a lock #88

Add a withClockSkewUpperBound option when acquiring a lock #88

tso commented Mar 10, 2023

qvissak Jun 19, 2023

qvissak Jun 19, 2023

qvissak Jun 19, 2023

qvissak Jun 20, 2023

qvissak Jun 23, 2023

qvissak Jun 23, 2023

qvissak Jun 23, 2023

qvissak Jun 23, 2023

qvissak Jun 26, 2023

qvissak Jun 29, 2023 •

edited

Loading

qvissak left a comment

qvissak Jun 29, 2023

qvissak Jun 29, 2023

tso Jun 29, 2023 •

edited

Loading

qvissak Jul 6, 2023

tso Jul 24, 2023

tso Jul 24, 2023

rahulvishwanatham commented Nov 1, 2023

nathanallen-toast commented Dec 27, 2023

lcabancla commented Feb 16, 2024

Add a withClockSkewUpperBound option when acquiring a lock #88

Are you sure you want to change the base?

Add a withClockSkewUpperBound option when acquiring a lock #88

Conversation

tso commented Mar 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qvissak Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

qvissak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tso Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahulvishwanatham commented Nov 1, 2023

nathanallen-toast commented Dec 27, 2023

lcabancla commented Feb 16, 2024

qvissak Jun 29, 2023 •

edited

Loading

tso Jun 29, 2023 •

edited

Loading