CURATOR-710: Fix leaking watch in EnsembleTracker by kezhuw · Pull Request #508 · apache/curator

kezhuw · 2024-10-16T12:54:02Z

CURATOR-667(#474) fixes asynchronous event path for getConfig to
"/zookeeper/config" by using CuratorFramework::usingNamespace(null) to
fetch data.

It causes watcher not registering to possible WatcherRemovalManager,
so leaking in WatcherRemoveCuratorFramework::removeWatchers.

CURATOR-667(apache#474) fixes asynchronous event path for `getConfig` to "/zookeeper/config" by using `CuratorFramework::usingNamespace(null)` to fetch data. It causes watcher not registering to possible `WatcherRemovalManager`, so leaking in `WatcherRemoveCuratorFramework::removeWatchers`.

tisonkun · 2024-11-03T05:45:15Z

+    Watching setWatcherRemovalManager(WatcherRemovalManager watcherRemovalManager) {
+        this.watcherRemovalManager = watcherRemovalManager;
+        return this;
+    }


It seems the watcherRemovalManager already set in the constructor by this.watcherRemovalManager = client.getWatcherRemovalManager();?

Or otherwise we can merge this setter into the constructor as a parameter.

tisonkun · 2024-11-03T05:47:58Z

-            if (client.getWatcherRemovalManager() != null) {
-                client.getWatcherRemovalManager().add(namespaceWatcher);
-            }
+        if (doCommit && namespaceWatcher != null && watcherRemovalManager != null) {


Can you elaborate a bit when watcherRemovalManager can be different from client.getWatcherRemovalManager().

I agree with the question: we can save some memory by not having the field. The performance impact is negligible

Given a fresh new CuratorFrameworkImpl, say framework0.

Let's assume framework1 = framework0.newWatcherRemoveCuratorFramework(). framework1.getWatcherRemovalManager will be the WatcherRemovalManager.

Let's assume framework2 = frame1.usingNamespace(null). framework2 will loss WatcherRemovalManager from above.

framework1.removeWatchers will not drop watchers from new Watching(framework2, ...).

Step.2 is what exactly EnsembleTracker does.

curator/curator-framework/src/main/java/org/apache/curator/framework/imps/EnsembleTracker.java

Lines 87 to 90 in 6a27ff0

EnsembleTracker(CuratorFramework client, EnsembleProvider ensembleProvider) {

this.client = client.newWatcherRemoveCuratorFramework();

this.ensembleProvider = ensembleProvider;

}

Step.3 is what exactly GetConfigBuilderImpl(CURATOR-667(#474)) does currently.

curator/curator-framework/src/main/java/org/apache/curator/framework/imps/GetConfigBuilderImpl.java

Lines 41 to 45 in 6a27ff0

public GetConfigBuilderImpl(CuratorFrameworkImpl client) {

this.client = (CuratorFrameworkImpl) client.usingNamespace(null);

backgrounding = new Backgrounding();

watching = new Watching(this.client);

}

Step.4 is where this bug emerges.

curator/curator-framework/src/main/java/org/apache/curator/framework/imps/EnsembleTracker.java

Lines 99 to 104 in 6a27ff0

public void close() {

if (state.compareAndSet(State.STARTED, State.CLOSED)) {

client.removeWatchers();

client.getConnectionStateListenable().removeListener(connectionStateListener);

}

}

I have pushed a fixup commit 6b78a3b with comments to doc this.

I get the point now. Let me check the code to see if we can have other less stateful/mutable solution 😆

Here is a patch to avoid the later set kezhuw#1.

And I agree that we need some effort for #517 to keep the inheritance hierarchy easier to discover and extend.

Briefly, the root cause is WatcherRemovalFacade::usingNamespace drop the removalManager. Then by convey the manager in the method, things should work well.

I saw the patch. I think it is what I am trying to express in #517 (comment).

whether CuratorFramework::usingNamespace should inherit functionalities of this ?

It changes the behavior of CuratorFramework::usingNamespace, but I am ok.

Yeah .. I suppose it's more like a bug to fix, since CURATOR-710 indicates that it's a bug anyway.

To provide an exit gate, perhaps we can add a new method to return a "basic CuratorFramework" to drop the removalManager explicitly, just like usingNamespace(null) to drop the namespace in facade. But we can leave it to #517.

tisonkun · 2024-12-17T16:03:02Z

Is this PR ready for review now? @kezhuw

kezhuw · 2024-12-19T09:51:40Z

@tisonkun It is ready. I have closed and reopened to revive ci.

Signed-off-by: tison <[email protected]>

avoid mutable state

kezhuw force-pushed the CURATOR-710-fix-EnsembleTracker-leak-watch branch from a6319f0 to b74abb0 Compare October 28, 2024 08:10

kezhuw force-pushed the CURATOR-710-fix-EnsembleTracker-leak-watch branch from b74abb0 to cd03c72 Compare October 28, 2024 08:14

kezhuw requested review from eolivelli and tisonkun November 2, 2024 15:48

tisonkun reviewed Nov 3, 2024

View reviewed changes

Comments a bit after review

6b78a3b

kezhuw closed this Dec 19, 2024

kezhuw reopened this Dec 19, 2024

kezhuw mentioned this pull request Dec 31, 2024

CURATOR-718: Refactor CuratorFramework inheritance hierarchy by composing functionalities #517

Merged

tisonkun and others added 2 commits January 17, 2025 00:12

avoid mutable state

5cf5e1c

Signed-off-by: tison <[email protected]>

Merge pull request #1 from tisonkun/patch

9cc6766

avoid mutable state

tisonkun approved these changes Jan 16, 2025

View reviewed changes

kezhuw merged commit fb78e23 into apache:master Jan 19, 2025

kezhuw mentioned this pull request Mar 12, 2025

CuratorFramework.close() is much slower in 5.8.0 when ZK server is down #1253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CURATOR-710: Fix leaking watch in EnsembleTracker#508

CURATOR-710: Fix leaking watch in EnsembleTracker#508
kezhuw merged 4 commits intoapache:masterfrom
kezhuw:CURATOR-710-fix-EnsembleTracker-leak-watch

kezhuw commented Oct 16, 2024

Uh oh!

tisonkun Nov 3, 2024

Uh oh!

tisonkun Nov 3, 2024

Uh oh!

tisonkun Nov 3, 2024

Uh oh!

eolivelli Nov 3, 2024

Uh oh!

kezhuw Nov 6, 2024

Uh oh!

tisonkun Jan 16, 2025

Uh oh!

tisonkun Jan 16, 2025 •

edited

Loading

Uh oh!

tisonkun Jan 16, 2025

Uh oh!

kezhuw Jan 16, 2025

Uh oh!

tisonkun Jan 16, 2025

Uh oh!

tisonkun commented Dec 17, 2024

Uh oh!

kezhuw commented Dec 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	EnsembleTracker(CuratorFramework client, EnsembleProvider ensembleProvider) {
	this.client = client.newWatcherRemoveCuratorFramework();
	this.ensembleProvider = ensembleProvider;
	}

	public GetConfigBuilderImpl(CuratorFrameworkImpl client) {
	this.client = (CuratorFrameworkImpl) client.usingNamespace(null);
	backgrounding = new Backgrounding();
	watching = new Watching(this.client);
	}

	public void close() {
	if (state.compareAndSet(State.STARTED, State.CLOSED)) {
	client.removeWatchers();
	client.getConnectionStateListenable().removeListener(connectionStateListener);
	}
	}

Conversation

kezhuw commented Oct 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tisonkun Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tisonkun commented Dec 17, 2024

Uh oh!

kezhuw commented Dec 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tisonkun Jan 16, 2025 •

edited

Loading