You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/05/18 21:08:02 UTC
[GitHub] [helix] narendly opened a new pull request #1016: Add null check in close() of ZkConnectionManager
narendly opened a new pull request #1016:
URL: https://github.com/apache/helix/pull/1016
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1015
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
Java objects may not have been fully initialized when their callback/interface methods are called. This adds a null check to ensure that an NPE does not happen.
### Tests
- [x] The following tests are written for this issue:
Simple null check. No explicit test needed.
### Commits
- [x] My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation (Optional)
- [ ] In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Code Quality
- [x] My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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
[GitHub] [helix] pkuwm commented on a change in pull request #1016: Add null check in close() of ZkConnectionManager
Posted by 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
[GitHub] [helix] narendly commented on pull request #1016: Add null check in close() of ZkConnectionManager
Posted by GitBox <gi...@apache.org>.
narendly commented on pull request #1016:
URL: https://github.com/apache/helix/pull/1016#issuecomment-630577706
This PR is ready to be merged, approved by @pkuwm
----------------------------------------------------------------
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
[GitHub] [helix] pkuwm commented on a change in pull request #1016: Add null check in close() of ZkConnectionManager
Posted by 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. The solution should be wait until connected in SharedZkClient if this connection is newly created.
```
connectionManager =
new ZkConnectionManager(createZkConnection(connectionConfig), connectInitTimeout,
connectionConfig.toString());
_connectionManagerPool.put(connectionConfig, connectionManager);
```
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
[GitHub] [helix] narendly merged pull request #1016: Add null check in close() of ZkConnectionManager
Posted by GitBox <gi...@apache.org>.
narendly merged pull request #1016:
URL: https://github.com/apache/helix/pull/1016
----------------------------------------------------------------
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
[GitHub] [helix] pkuwm commented on a change in pull request #1016: Add null check in close() of ZkConnectionManager
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1016:
URL: https://github.com/apache/helix/pull/1016#discussion_r426967132
##########
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:
Synced offline. I missed the point ZkConnectionManager is managing connection. So there is a wait in ZkClient.
This NPE is thrown when ZkConnectionManager is being constructed and connection is not able to connect to ZK within timeout.
----------------------------------------------------------------
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