[ https://issues.apache.org/jira/browse/ZOOKEEPER-2778?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16693898#comment-16693898 ]
Michael K. Edwards edited comment on ZOOKEEPER-2778 at 11/20/18 11:41 PM: -------------------------------------------------------------------------- >From what I'm seeing, it would be a crashing bug for `getQuorumAddress()` >(which cannot be marked `protected`, because it's called by has-a holders of a >QuorumPeer reference rather than by is-a subclasses of QuorumPeer, but can and >should be package-private) to be called before the addresses are set. The >only call to `getClientAddress()` (which should be `private`) is in >`processReconfig()`, and it's appropriate for it to return `null` if called >early. This leaves `getElectionAddress()`, which again is pseudo-protected >and would produce a crash if called before the addresses are set. So the actual problem here is that, if the election address is not yet known, there's no safe return value from `getElectionAddress()` in the race scenario cited in the bug description. This "fix" – hanm's or mine – will turn it into an NPE instead of a deadlock. This might be addressable by ensuring that code that needs the `QV_LOCK` for a `QuorumPeer` associated with a `QuorumCnxManager` (to protect the macroscopic critical sections in `QuorumCnxManager.connectOne()`, `QuorumPeer.setLastSeenQuorumVerifier()`, and `QuorumPeer.setQuorumVerifier()`) always takes the lock on the `QuorumCnxManager` instance first. Looking into that. was (Author: mkedwards): >From what I'm seeing, it would be a crashing bug for `getQuorumAddress()` >(which cannot be marked `protected`, because it's called by has-a holders of a >QuorumPeer reference rather than by is-a subclasses of QuorumPeer, but can and >should be package-private) to be called before the addresses are set. The >only call to `getClientAddress()` (which should be `private`) is in >`processReconfig()`, and it's appropriate for it to return `null` if called >early. This leaves `getElectionAddress()`, which again is pseudo-protected >and would produce a crash if called before the addresses are set. So the actual problem here is that, if the election address is not yet known, there's no safe return value from `getElectionAddress()` in the race scenario cited in the bug description. This "fix" – hanm's or mine – will turn it into an NPE instead of a deadlock. This might be addressable by ensuring that code that needs the `QV_LOCK` for a `QuorumPeer` associated with a `QuorumCnxManager` (to protect the macroscopic critical sections in `QuorumCnxManager.connectOne()`, `QuorumPeer.setLastSeenQuorumVerifier()`, and `QuorumPeer.setQuorumVerifier()`) always takes the lock on the `QuorumCnxManager` instance first. Looking into that. > Potential server deadlock between follower sync with leader and follower > receiving external connection requests. > ---------------------------------------------------------------------------------------------------------------- > > Key: ZOOKEEPER-2778 > URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2778 > Project: ZooKeeper > Issue Type: Bug > Components: quorum > Affects Versions: 3.5.3 > Reporter: Michael Han > Priority: Blocker > Labels: pull-request-available > Fix For: 3.6.0, 3.5.5 > > Time Spent: 1h 20m > Remaining Estimate: 0h > > It's possible to have a deadlock during recovery phase. > Found this issue by analyzing thread dumps of "flaky" ReconfigRecoveryTest > [1]. . Here is a sample thread dump that illustrates the state of the > execution: > {noformat} > [junit] java.lang.Thread.State: BLOCKED > [junit] at > org.apache.zookeeper.server.quorum.QuorumPeer.getElectionAddress(QuorumPeer.java:686) > [junit] at > org.apache.zookeeper.server.quorum.QuorumCnxManager.initiateConnection(QuorumCnxManager.java:265) > [junit] at > org.apache.zookeeper.server.quorum.QuorumCnxManager.connectOne(QuorumCnxManager.java:445) > [junit] at > org.apache.zookeeper.server.quorum.QuorumCnxManager.receiveConnection(QuorumCnxManager.java:369) > [junit] at > org.apache.zookeeper.server.quorum.QuorumCnxManager$Listener.run(QuorumCnxManager.java:642) > [junit] > [junit] java.lang.Thread.State: BLOCKED > [junit] at > org.apache.zookeeper.server.quorum.QuorumCnxManager.connectOne(QuorumCnxManager.java:472) > [junit] at > org.apache.zookeeper.server.quorum.QuorumPeer.connectNewPeers(QuorumPeer.java:1438) > [junit] at > org.apache.zookeeper.server.quorum.QuorumPeer.setLastSeenQuorumVerifier(QuorumPeer.java:1471) > [junit] at > org.apache.zookeeper.server.quorum.Learner.syncWithLeader(Learner.java:520) > [junit] at > org.apache.zookeeper.server.quorum.Follower.followLeader(Follower.java:88) > [junit] at > org.apache.zookeeper.server.quorum.QuorumPeer.run(QuorumPeer.java:1133) > {noformat} > The dead lock happens between the quorum peer thread which running the > follower that doing sync with leader work, and the listener of the qcm of the > same quorum peer that doing the receiving connection work. Basically to > finish sync with leader, the follower needs to synchronize on both QV_LOCK > and the qmc object it owns; while in the receiver thread to finish setup an > incoming connection the thread needs to synchronize on both the qcm object > the quorum peer owns, and the same QV_LOCK. It's easy to see the problem here > is the order of acquiring two locks are different, thus depends on timing / > actual execution order, two threads might end up acquiring one lock while > holding another. > [1] > org.apache.zookeeper.server.quorum.ReconfigRecoveryTest.testCurrentServersAreObserversInNextConfig -- This message was sent by Atlassian JIRA (v7.6.3#76005)