pkuwm commented on a change in pull request #1016:
URL: https://github.com/apache/helix/pull/1016#discussion_r426932108
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/ZkConnectionManager.java
##########
@@ -118,7 +118,7 @@ public void close() {
protected synchronized void close(boolean skipIfWatched) {
cleanupInactiveWatchers();
- if (_sharedWatchers.size() > 0) {
+ if (_sharedWatchers != null && _sharedWatchers.size() > 0) {
Review comment:
It seems `_shareWatchers` is initialized outside the constructor of
`ZkConnectionManager`. If `ZkConnectionManager` completes initialization,
`_sharedWatchers` should also complete initialization.
Doing the null check is unnecessary: an instance should be constructed well
before it is used. `close()` should be called after the instance completes
init. Why close() is called before the instance's state is fully initialized?
We should find it out.
In one case I've known, it is related to SharedZkClient. The ZkConnection is
created within sharedZkClient, before the ZK session is established, it is
passed to ZkConnectionManager and ZkClient's connect(). And in ZkClient's
connect, this is a check for this shared connection. Because the session is not
established, zkclient has to `close()`. Meanwhile, `_shareWatchers` is not
initialized.
I think we should fix the root cause, instead of covering the problem with
this null check. what do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
