Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CURATOR-710: Fix leaking watch in EnsembleTracker #508

Merged
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
avoid mutable state
Signed-off-by: tison <wander4096@gmail.com>
tisonkun committed Jan 16, 2025

Verified

This commit was signed with the committer’s verified signature.
tisonkun tison
commit 5cf5e1c15e7741e44a938160c601638757eb59d4
Original file line number Diff line number Diff line change
@@ -33,7 +33,6 @@
public class GetConfigBuilderImpl
implements GetConfigBuilder, BackgroundOperation<Void>, ErrorListenerEnsembleable<byte[]> {
private final CuratorFrameworkImpl client;
private final WatcherRemovalManager watcherRemovalManager;

private Backgrounding backgrounding;
private Watching watching;
@@ -45,14 +44,8 @@ public GetConfigBuilderImpl(CuratorFrameworkImpl client) {

public GetConfigBuilderImpl(CuratorFrameworkImpl client, Backgrounding backgrounding, Watcher watcher, Stat stat) {
this.client = (CuratorFrameworkImpl) client.usingNamespace(null);
this.watcherRemovalManager = client.getWatcherRemovalManager();
this.backgrounding = backgrounding;
// We are using `client.usingNamespace(null)` to avoid `unfixNamespace` for "/zookeeper/config"(CURATOR-667)
// events. But `client.usingNamespace(null)` will loss possible `WatcherRemovalManager`(CURATOR-710). So, let's
// reset it.
//
// See also `NamespaceWatchedEvent`.
this.watching = new Watching(this.client, watcher).setWatcherRemovalManager(watcherRemovalManager);
this.watching = new Watching(this.client, watcher);
this.stat = stat;
}

@@ -115,19 +108,19 @@ public BackgroundEnsembleable<byte[]> usingWatcher(CuratorWatcher watcher) {

@Override
public BackgroundEnsembleable<byte[]> watched() {
watching = new Watching(client, true).setWatcherRemovalManager(watcherRemovalManager);
watching = new Watching(client, true);
return new InternalBackgroundEnsembleable();
}

@Override
public BackgroundEnsembleable<byte[]> usingWatcher(Watcher watcher) {
watching = new Watching(client, watcher).setWatcherRemovalManager(watcherRemovalManager);
watching = new Watching(client, watcher);
return new InternalBackgroundEnsembleable();
}

@Override
public BackgroundEnsembleable<byte[]> usingWatcher(CuratorWatcher watcher) {
watching = new Watching(client, watcher).setWatcherRemovalManager(watcherRemovalManager);
watching = new Watching(client, watcher);
return new InternalBackgroundEnsembleable();
}

Original file line number Diff line number Diff line change
@@ -37,9 +37,13 @@ class WatcherRemovalFacade extends CuratorFrameworkImpl implements WatcherRemove
private final WatcherRemovalManager removalManager;

WatcherRemovalFacade(CuratorFrameworkImpl client) {
this(client, new WatcherRemovalManager(client));
}

private WatcherRemovalFacade(CuratorFrameworkImpl client, WatcherRemovalManager removalManager) {
super(client);
this.client = client;
removalManager = new WatcherRemovalManager(client);
this.removalManager = removalManager;
}

@Override
@@ -73,7 +77,8 @@ public CuratorFramework nonNamespaceView() {

@Override
public CuratorFramework usingNamespace(String newNamespace) {
return client.usingNamespace(newNamespace);
final CuratorFrameworkImpl newClient = (CuratorFrameworkImpl) client.usingNamespace(newNamespace);
return new WatcherRemovalFacade(newClient, removalManager);
}

@Override
Original file line number Diff line number Diff line change
@@ -28,46 +28,36 @@ public class Watching {
private final CuratorWatcher curatorWatcher;
private final boolean watched;
private final CuratorFrameworkImpl client;
private WatcherRemovalManager watcherRemovalManager;
private NamespaceWatcher namespaceWatcher;

public Watching(CuratorFrameworkImpl client, boolean watched) {
this.client = client;
this.watcherRemovalManager = client.getWatcherRemovalManager();
this.watcher = null;
this.curatorWatcher = null;
this.watched = watched;
}

public Watching(CuratorFrameworkImpl client, Watcher watcher) {
this.client = client;
this.watcherRemovalManager = client.getWatcherRemovalManager();
this.watcher = watcher;
this.curatorWatcher = null;
this.watched = false;
}

public Watching(CuratorFrameworkImpl client, CuratorWatcher watcher) {
this.client = client;
this.watcherRemovalManager = client.getWatcherRemovalManager();
this.watcher = null;
this.curatorWatcher = watcher;
this.watched = false;
}

public Watching(CuratorFrameworkImpl client) {
this.client = client;
this.watcherRemovalManager = client.getWatcherRemovalManager();
watcher = null;
watched = false;
curatorWatcher = null;
}

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

Watcher getWatcher(String unfixedPath) {
namespaceWatcher = null;
if (watcher != null) {
@@ -95,8 +85,8 @@ void commitWatcher(int rc, boolean isExists) {
doCommit = (rc == KeeperException.Code.OK.intValue());
}

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