[GitHub] [helix] pkuwm commented on a change in pull request #1016: Add null check in close() of ZkConnectionManager

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org