ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-696

NPE in the hudson logs, seems nioservercnxn closed twice

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.3.0
    • Component/s: server
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      seeing the following on the console for http://hudson.zones.apache.org/hudson/view/ZooKeeper/job/ZooKeeper-trunk/729/

      looks like the cnxn is closed twice? (the second time 'sock' is null). perhaps it's due to client closing and sending session term, then closing socket, server sees the read return -1, so closes cnxn, then sees the session close request (which was queued)?

      [junit] 2010-03-10 03:15:53,205 - INFO [main:NIOServerCnxn@1232] - Closed socket connection for client /127.0.0.1:41285 which had sessionid 0x127461233fc0000
      [junit] 2010-03-10 03:15:53,206 - WARN [NIOServerCxn.Factory:0.0.0.0/0.0.0.0:11221:NIOServerCnxn$Factory@269] - Ignoring unexpected runtime exception
      [junit] java.lang.NullPointerException
      [junit] at org.apache.zookeeper.server.NIOServerCnxn.close(NIOServerCnxn.java:1232)
      [junit] at org.apache.zookeeper.server.NIOServerCnxn.doIO(NIOServerCnxn.java:594)
      [junit] at org.apache.zookeeper.server.NIOServerCnxn$Factory.run(NIOServerCnxn.java:259)

        Activity

        Hide
        Patrick Hunt added a comment -

        Looks like the issue is that in the tests we are both shutting down the clients and also shutting down the server. As a result in some cases the socket is nulled by one/other of the close calls while the other close call then sees a null for the sock and NPEs. I'll work on a patch.

        Show
        Patrick Hunt added a comment - Looks like the issue is that in the tests we are both shutting down the clients and also shutting down the server. As a result in some cases the socket is nulled by one/other of the close calls while the other close call then sees a null for the sock and NPEs. I'll work on a patch.
        Hide
        Patrick Hunt added a comment -

        this patch removes all npes in the log on my machine

        1) close could be called from multiple threads (in case of testing). I changed the synchronization quite a bit here so pay close attention
        2) notice that clear and other methods calling close on the cnxns list now clone the list and call close, close removes the cnxn from the list itself
        3) notice that close determines at the start whether the cnxn is closed yet or not - based on whether the cnxn is in the cnxns list or not
        4) sessiontest - fixed unrelated npe that was showing up
        5) leader - this code looked wrong to me. Notice that ss close was after the cnxns close, seems to me that this should be first to eliminate the possibility that someone added to the cnxns after that list had been cleared (factory run method checks if ss is closed or not and exits if it is)

        would be good to have more than one pair of eyes on this review

        Show
        Patrick Hunt added a comment - this patch removes all npes in the log on my machine 1) close could be called from multiple threads (in case of testing). I changed the synchronization quite a bit here so pay close attention 2) notice that clear and other methods calling close on the cnxns list now clone the list and call close, close removes the cnxn from the list itself 3) notice that close determines at the start whether the cnxn is closed yet or not - based on whether the cnxn is in the cnxns list or not 4) sessiontest - fixed unrelated npe that was showing up 5) leader - this code looked wrong to me. Notice that ss close was after the cnxns close, seems to me that this should be first to eliminate the possibility that someone added to the cnxns after that list had been cleared (factory run method checks if ss is closed or not and exits if it is) would be good to have more than one pair of eyes on this review
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12438453/ZOOKEEPER-696.patch
        against trunk revision 921509.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/138/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/138/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/138/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12438453/ZOOKEEPER-696.patch against trunk revision 921509. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/138/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/138/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/138/console This message is automatically generated.
        Hide
        Flavio Junqueira added a comment -

        +1, it looks good to me.

        Show
        Flavio Junqueira added a comment - +1, it looks good to me.
        Hide
        Mahadev konar added a comment -

        +1 the patch looks good!

        Show
        Mahadev konar added a comment - +1 the patch looks good!
        Hide
        Mahadev konar added a comment -

        I just committed this. thanks pat!

        Show
        Mahadev konar added a comment - I just committed this. thanks pat!
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #732 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/732/)
        . NPE in the hudson logs, seems nioservercnxn closed twice (phunt via mahadev)

        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #732 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/732/ ) . NPE in the hudson logs, seems nioservercnxn closed twice (phunt via mahadev)

          People

          • Assignee:
            Patrick Hunt
            Reporter:
            Patrick Hunt
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development