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

Potential NPE in Observer#observeLeader and Follower#followLeader #504

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Potential NPE in Observer#observeLeader and Follower#followLeader #504

wants to merge 6 commits into from

Conversation

brettKK
Copy link

@brettKK brettKK commented Apr 22, 2018

@lj1043041006 found a potential NPE in ZK

Bug description:

callee Learner#findLeader will return null and callee developer check it but just log:

// code placeholder
if (leaderServer == null) {
   LOG.warn("Couldn't find the leader with id = " + current.getId());
}
return leaderServer;

caller Observer#observeLeader and Follower#followLeader will directly use return value w/o null check:

//Follower#followLeader
QuorumServer leaderServer = findLeader();
try {
    connectToLeader(leaderServer.addr, leaderServer.hostname);
    ..........
}
//Observer#observeLeader
QuorumServer leaderServer = findLeader();
LOG.info("Observing " + leaderServer.addr);
try {
    connectToLeader(leaderServer.addr, leaderServer.hostname);
}

https://issues.apache.org/jira/browse/ZOOKEEPER-3010

@lvfangmin
Copy link
Contributor

The newly added exception will be caught in the QuorumPeer, which will trigger another round of look up.

This change looks good to me, it’s always safer to check, although this may not actually happen on 3.6 with dynamic config, unless there is a bug, which we should fix.

+1

@anmolnar
Copy link
Contributor

@brettKK Are you still working on this patch?
Would you please rebase?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants