Repository navigation
GH-1170: Fix watcher leak in DistributedBarrier#waitOnBarrier - #1312
Open
Sanskar121543 wants to merge 1 commit into
Open
Sanskar121543 wants to merge 1 commit into
Sanskar121543 wants to merge 1 commit into
Conversation
Each iteration of the wait loop registers a new exists-watch on the barrier path via usingWatcher(). When the barrier node doesn't exist, or when maxWait elapses before the watch fires, waitOnBarrier() returned without ever removing that watch. In a system that creates and waits on many DistributedBarriers, those dangling watches accumulate on both the client and the server without bound, which can eventually exhaust memory. Remove the watch waitOnBarrier() just registered before returning, in a finally block so it runs on every exit path (barrier already gone, maxWait elapsed, or an exception). Removal is quiet: if the watch already fired (the common, successful case), there's nothing left to remove and this is a no-op.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes #1170 (CURATOR-654).
DistributedBarrier#waitOnBarrierregisters a ZK exists-watch on the barrier path on every loop iteration viaclient.checkExists().usingWatcher(watcher).forPath(barrierPath), but never removes it. In particular:waitOnBarrier()is called,checkExists()still registers the watch (ZooKeeper sets exists-watches regardless of whether the node is currently present, so it can notify on creation), but the method returns immediately (result == true) without the watch ever firing.maxWaitelapses before the node is removed, the method returns (false) with the last-registered watch still outstanding.In a system that creates and waits on
DistributedBarriers repeatedly (the scenario described in the original report), each such call leaves behind a dangling watch that is never cleaned up, client- and server-side, until the watched node happens to be created/changed - or never, which is how this leads to unbounded memory growth over time.This PR was previously attempted in #435, which was closed as inactive; I picked it up as suggested in the issue thread, using Curator's current watch-removal API instead of that PR's approach.
Fix
Wrap the wait loop in a
try/finallyand remove the watchwaitOnBarrier()just registered before returning, usingclient.watches().remove(watcher).quietly().forPath(barrierPath)..quietly()suppressesNoWatcherException, so this is a safe no-op on the common/successful path where the watch already fired (e.g. the barrier was removed andcheckExists()returned null because of that event, not because the node was never there). Any other failure during cleanup (e.g. connection issues) is caught and logged at debug level rather than propagated, so it can't mask the real result ofwaitOnBarrier()or swallow an exception already in flight.Testing
Added
testNoBarrierDoesNotLeakWatchtoTestDistributedBarrier, which:waitOnBarrier()on a barrier path that doesn't exist (same setup as the existingtestNoBarriertest).DistributedBarrier's privatewatchervia reflection.client.watches().remove(watcher).forPath(path)(without.quietly()) throwsKeeperException.NoWatcherException- i.e. there's nothing left to remove, becausewaitOnBarrier()already cleaned it up.I verified this test fails against the unpatched code (the removal succeeds instead of throwing, because the watch is still dangling) and passes with the fix. Ran the full
TestDistributedBarriersuite (5/5 passing) against a real embedded ZooKeeper viaBaseClassForTests/TestingServer- no broker mocking involved.