zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mkedwards <...@git.apache.org>
Subject [GitHub] zookeeper pull request #707: [ZOOKEEPER-2778] QuorumPeer: encapsulate addres...
Date Wed, 21 Nov 2018 16:36:55 GMT
Github user mkedwards commented on a diff in the pull request:

    https://github.com/apache/zookeeper/pull/707#discussion_r235460740
  
    --- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeer.java
---
    @@ -1556,32 +1573,50 @@ public String getNextDynamicConfigFilename() {
         }
         
         public void setLastSeenQuorumVerifier(QuorumVerifier qv, boolean writeToDisk){
    -        synchronized (QV_LOCK) {
    -            if (lastSeenQuorumVerifier != null && lastSeenQuorumVerifier.getVersion()
> qv.getVersion()) {
    -                LOG.error("setLastSeenQuorumVerifier called with stale config " + qv.getVersion()
+
    -                        ". Current version: " + quorumVerifier.getVersion());
    +        // If qcm is non-null, we may call qcm.connectOne(), which will take the lock
on qcm
    +        // and then take QV_LOCK.  Take the locks in the same order to ensure that we
don't
    +        // deadlock against other callers of connectOne().  If qcmRef gets set in another
    +        // thread while we're inside the synchronized block, that does no harm; if we
didn't
    +        // take a lock on qcm (because it was null when we sampled it), we won't call
    +        // connectOne() on it.  (Use of an AtomicReference is enough to guarantee visibility
    +        // of updates that provably happen in another thread before entering this method.)
    +        QuorumCnxManager qcm = qcmRef.get();
    +        Object outerLockObject = (qcm != null) ? qcm : QV_LOCK;
    --- End diff --
    
    Yes; I'd like to get confirmation from @castuardo and his team that this fully solves
the lock nesting problem.  But if you look at the flow of the code, I think you'll see that
the only way _this_ code can take the lock on a {{QuorumCnxManager}} is via {{qcm.connectOne()}}.
 Since Java locks are reentrant, we're safe ensuring that the nesting is always qcm ->
QV_LOCK -> qcm -> QV_LOCK if qcm is non-null and QV_LOCK -> QV_LOCK if qcm is null.


---

Mime
View raw message