[fix][meta] Fixed race condition between ResourceLock update and invalidation #19817
Conversation
…e and invalidation
merlimat
added
type/bug
merlimat
changed the title
|
/pulsarbot run-failure-checks |
| pendingOperationFuture = pendingOperationFuture.thenCompose(v -> { | ||
| synchronized (ResourceLockImpl.this) { | ||
| if (state != State.Valid) { | ||
| return CompletableFuture.failedFuture( |
There was a problem hiding this comment.
We have returned a new exception. Do we need to change the relevant exception catch?
| return acquire(newValue); | ||
| // If there is an operation in progress, we're going to let it complete before attempting to | ||
| // update the value | ||
| pendingOperationFuture = pendingOperationFuture.thenCompose(v -> { |
There was a problem hiding this comment.
pendingOperationFuture may throw exceptions LockBusyException, BadVersionException or other connectivity issues and return errors directly. Do we need to check that the relevant usage logic has handled this exception?
There was a problem hiding this comment.
That's correct, I think we're missing to reset it once it gets completed once.
|
|
| revalidateFuture.whenComplete((unused, throwable) -> { | ||
| doRevalidate(newValue).thenRun(() -> newFuture.complete(null)) | ||
| trackFuture = new CompletableFuture<>(); | ||
| trackFuture.whenComplete((unused, throwable) -> { |
There was a problem hiding this comment.
It is wrong... the track future will always be incomplete. :(
There was a problem hiding this comment.
#19844 is trying to perfect it.
… update and invalidation (#19844)
… update and invalidation (#19859)
Motivation
The test
LockManagerTest.updateValueWhenKeyDisappears()can having infrequent failures, especially when running with Etcd backend.The problem is really in the logic of the
ResourceLockitself. After a normal lock-acquire operation, the test is triggering 2 events at the same time:lock.update()operation to change the value of the lock.The race condition is due to the fact that operation (1) will trigger a lock revalidation in background and this now happens at the same time as operation (2).
The result is that it's possible for the ResourceLock to enter into a loop of revalidations and deletions.
eg:
Modifications
Similarly as to what we're already doing to prevent concurrent revalidate operations, we need to also wait for any existing operations to complete before attempting to do the update.
The same is true if there is an update() operation in progress: the next revalidate should wait.
We then use a single future to signal pending operation and to piggy back and try when it's the right time.
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: