-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
CURATOR-710: Fix leaking watch in EnsembleTracker #508
Conversation
a6319f0
to
b74abb0
Compare
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`.
b74abb0
to
cd03c72
Compare
Watching setWatcherRemovalManager(WatcherRemovalManager watcherRemovalManager) { | ||
this.watcherRemovalManager = watcherRemovalManager; | ||
return this; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the watcherRemovalManager
already set in the constructor by this.watcherRemovalManager = client.getWatcherRemovalManager();
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or otherwise we can merge this setter into the constructor as a parameter.
if (client.getWatcherRemovalManager() != null) { | ||
client.getWatcherRemovalManager().add(namespaceWatcher); | ||
} | ||
if (doCommit && namespaceWatcher != null && watcherRemovalManager != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate a bit when watcherRemovalManager
can be different from client.getWatcherRemovalManager()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the question: we can save some memory by not having the field. The performance impact is negligible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Given a fresh new
CuratorFrameworkImpl
, sayframework0
. - Let's assume
framework1 = framework0.newWatcherRemoveCuratorFramework()
.framework1.getWatcherRemovalManager
will be theWatcherRemovalManager
. - Let's assume
framework2 = frame1.usingNamespace(null)
.framework2
will lossWatcherRemovalManager
from above. framework1.removeWatchers
will not drop watchers fromnew 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the point now. Let me check the code to see if we can have other less stateful/mutable solution 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Briefly, the root cause is WatcherRemovalFacade::usingNamespace
drop the removalManager
. Then by convey the manager in the method, things should work well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Is this PR ready for review now? @kezhuw |
@tisonkun It is ready. I have closed and reopened to revive ci. |
Signed-off-by: tison <[email protected]>
avoid mutable state
CURATOR-667(#474) fixes asynchronous event path for
getConfig
to"/zookeeper/config" by using
CuratorFramework::usingNamespace(null)
tofetch data.
It causes watcher not registering to possible
WatcherRemovalManager
,so leaking in
WatcherRemoveCuratorFramework::removeWatchers
.