Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.3.0
    • Component/s: server
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      not a blocker for 3.2, moving to 3.3

      Description

      There are two synchronized blocks locking on different objects, and to me they should be guarded by the same object. Here are the parts of the code I'm talking about:

      NIOServerCnxn.readRequest@444
      ...
                synchronized (this) {
                      outstandingRequests++;
                      // check throttling
                      if (zk.getInProcess() > factory.outstandingLimit) {
                          disableRecv();
                          // following lines should not be needed since we are already
                          // reading
                          // } else {
                          // enableRecv();
                      }
                  } 
      
      NIOServerCnxn.sendResponse@740
      ...
               synchronized (this.factory) {
                      outstandingRequests--;
                      // check throttling
                      if (zk.getInProcess() < factory.outstandingLimit
                              || outstandingRequests < 1) {
                          sk.selector().wakeup();
                          enableRecv();
                      }
                  }
      

      I think the second one is correct, and the first synchronized block should be guarded by "this.factory".

      This could be related to issue ZOOKEEPER-57, but I have no concrete indication that this is the case so far.

      1. zk-deadlock.png
        24 kB
        Todd Lipcon
      2. ZOOKEEPER-59.patch
        4 kB
        Flavio Junqueira
      3. ZOOKEEPER-59.patch
        4 kB
        Flavio Junqueira
      4. ZOOKEEPER-59.patch
        5 kB
        Flavio Junqueira

        Activity

        Flavio Junqueira created issue -
        Flavio Junqueira made changes -
        Field Original Value New Value
        Summary Synchornized block in NIOServerCnxn Synchronized block in NIOServerCnxn
        Description There are two synchronized blocks locking on different objects, and to me they should be guarded by the same object. Here are the parts of the code I'm talking about:

        NIOServerCnxn.readRequest@444
        ...
                  synchronized (this) {
                        outstandingRequests++;
                        // check throttling
                        if (zk.getInProcess() > factory.outstandingLimit) {
                            disableRecv();
                            // following lines should not be needed since we are already
                            // reading
                            // } else {
                            // enableRecv();
                        }
                    }


        NIOServerCnxn.sendResponse@740
        ...
                 synchronized (this.factory) {
                        outstandingRequests--;
                        // check throttling
                        if (zk.getInProcess() < factory.outstandingLimit
                                || outstandingRequests < 1) {
                            sk.selector().wakeup();
                            enableRecv();
                        }
                    }


        I think the second one is correct, and the first synchronized block should be guarded by "this.factory".

        This could be related to issue ZOOKEEPER-57, but I have no concrete indication that this is the case so far.
        There are two synchronized blocks locking on different objects, and to me they should be guarded by the same object. Here are the parts of the code I'm talking about:

        {quote}
        NIOServerCnxn.readRequest@444
        ...
                  synchronized (this) {
                        outstandingRequests++;
                        // check throttling
                        if (zk.getInProcess() > factory.outstandingLimit) {
                            disableRecv();
                            // following lines should not be needed since we are already
                            // reading
                            // } else {
                            // enableRecv();
                        }
                    }
        {quote}

        {quote}
        NIOServerCnxn.sendResponse@740
        ...
                 synchronized (this.factory) {
                        outstandingRequests--;
                        // check throttling
                        if (zk.getInProcess() < factory.outstandingLimit
                                || outstandingRequests < 1) {
                            sk.selector().wakeup();
                            enableRecv();
                        }
                    }
        {quote}

        I think the second one is correct, and the first synchronized block should be guarded by "this.factory".

        This could be related to issue ZOOKEEPER-57, but I have no concrete indication that this is the case so far.
        Flavio Junqueira made changes -
        Description There are two synchronized blocks locking on different objects, and to me they should be guarded by the same object. Here are the parts of the code I'm talking about:

        {quote}
        NIOServerCnxn.readRequest@444
        ...
                  synchronized (this) {
                        outstandingRequests++;
                        // check throttling
                        if (zk.getInProcess() > factory.outstandingLimit) {
                            disableRecv();
                            // following lines should not be needed since we are already
                            // reading
                            // } else {
                            // enableRecv();
                        }
                    }
        {quote}

        {quote}
        NIOServerCnxn.sendResponse@740
        ...
                 synchronized (this.factory) {
                        outstandingRequests--;
                        // check throttling
                        if (zk.getInProcess() < factory.outstandingLimit
                                || outstandingRequests < 1) {
                            sk.selector().wakeup();
                            enableRecv();
                        }
                    }
        {quote}

        I think the second one is correct, and the first synchronized block should be guarded by "this.factory".

        This could be related to issue ZOOKEEPER-57, but I have no concrete indication that this is the case so far.
        There are two synchronized blocks locking on different objects, and to me they should be guarded by the same object. Here are the parts of the code I'm talking about:

        {noformat}
        NIOServerCnxn.readRequest@444
        ...
                  synchronized (this) {
                        outstandingRequests++;
                        // check throttling
                        if (zk.getInProcess() > factory.outstandingLimit) {
                            disableRecv();
                            // following lines should not be needed since we are already
                            // reading
                            // } else {
                            // enableRecv();
                        }
                    }
        {noformat}

        {noformat}
        NIOServerCnxn.sendResponse@740
        ...
                 synchronized (this.factory) {
                        outstandingRequests--;
                        // check throttling
                        if (zk.getInProcess() < factory.outstandingLimit
                                || outstandingRequests < 1) {
                            sk.selector().wakeup();
                            enableRecv();
                        }
                    }
        {noformat}

        I think the second one is correct, and the first synchronized block should be guarded by "this.factory".

        This could be related to issue ZOOKEEPER-57, but I have no concrete indication that this is the case so far.
        Flavio Junqueira made changes -
        Assignee Flavio Paiva Junqueira [ fpj ]
        Flavio Junqueira made changes -
        Attachment ZOOKEEPER-59_1.patch [ 12385480 ]
        Flavio Junqueira made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Assignee Flavio Paiva Junqueira [ fpj ] Benjamin Reed [ breed ]
        Mahadev konar made changes -
        Attachment ZOOKEEPER-59_2.patch [ 12385524 ]
        Patrick Hunt made changes -
        Assignee Benjamin Reed [ breed ] Mahadev konar [ mahadev ]
        Status Patch Available [ 10002 ] Open [ 1 ]
        Flavio Junqueira made changes -
        Attachment ZOOKEEPER-59_1.patch [ 12385480 ]
        Flavio Junqueira made changes -
        Attachment ZOOKEEPER-59_2.patch [ 12385524 ]
        Flavio Junqueira made changes -
        Attachment ZOOKEEPER-59.patch [ 12386904 ]
        Flavio Junqueira made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Patrick Hunt made changes -
        Assignee Mahadev konar [ mahadev ] Flavio Paiva Junqueira [ fpj ]
        Patrick Hunt made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Flavio Junqueira made changes -
        Attachment ZOOKEEPER-59.patch [ 12386904 ]
        Flavio Junqueira made changes -
        Attachment ZOOKEEPER-59.patch.patch [ 12387314 ]
        Flavio Junqueira made changes -
        Attachment ZOOKEEPER-59.patch.patch [ 12387314 ]
        Flavio Junqueira made changes -
        Attachment ZOOKEEPER-59.patch [ 12387315 ]
        Patrick Hunt made changes -
        Fix Version/s 3.2.0 [ 12313491 ]
        Patrick Hunt made changes -
        Release Note not a blocker for 3.2, moving to 3.3
        Fix Version/s 3.3.0 [ 12313976 ]
        Fix Version/s 3.2.0 [ 12313491 ]
        Flavio Junqueira made changes -
        Attachment ZOOKEEPER-59.patch [ 12437656 ]
        Flavio Junqueira made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Flavio Junqueira made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Flavio Junqueira made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Patrick Hunt made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Patrick Hunt made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Flavio Junqueira made changes -
        Status Patch Available [ 10002 ] Open [ 1 ]
        Flavio Junqueira made changes -
        Attachment ZOOKEEPER-59.patch [ 12438130 ]
        Flavio Junqueira made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Benjamin Reed made changes -
        Hadoop Flags [Reviewed]
        Mahadev konar made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Todd Lipcon made changes -
        Attachment zk-deadlock.png [ 12438785 ]
        Patrick Hunt made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Flavio Junqueira
            Reporter:
            Flavio Junqueira
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development