Details

      Description

      This is basically the same issue as ZOOKEEPER-888 and ZOOKEEPER-740 (the latter is open as I write this, but it was superseded by the fix that went in with 888). The problem still exists after the ZOOKEEPER-888 patch, however; it's just more difficult to trigger:

      1) Zookeeper notices connection loss, schedules watcher_dispatch
      2) Zookeeper notices session loss, schedules watcher_dispatch
      3) watcher_dispatch runs for connection loss
      4) pywatcher is freed due to is_unrecoverable being true
      5) watcher_dispatch runs for session loss
      6) PyObject_CallObject attempts to run freed pywatcher with varying bad results

      The fix is easy, the dispatcher should act on the state it is given, not the state of the world when it runs. (Patch attached). Reliably triggering the crash is tricky due to the race, but it's not theoretical.

        Activity

        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #1513 (See https://builds.apache.org/job/ZooKeeper-trunk/1513/)
        ZOOKEEPER-1395. node-watcher double-free redux (Mike Lundy via henryr) (Revision 1307644)

        Result = SUCCESS
        henry : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1307644
        Files :

        • /zookeeper/trunk/CHANGES.txt
        • /zookeeper/trunk/src/contrib/zkpython/src/c/zookeeper.c
        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #1513 (See https://builds.apache.org/job/ZooKeeper-trunk/1513/ ) ZOOKEEPER-1395 . node-watcher double-free redux (Mike Lundy via henryr) (Revision 1307644) Result = SUCCESS henry : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1307644 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/contrib/zkpython/src/c/zookeeper.c
        Hide
        Henry Robinson added a comment -

        I just committed this to 3.3, 3.4 and 3.5. Thanks Mike!

        Show
        Henry Robinson added a comment - I just committed this to 3.3, 3.4 and 3.5. Thanks Mike!
        Hide
        Henry Robinson added a comment -

        +1 this looks sensible to me. I'll commit tonight or tomorrow.

        Show
        Henry Robinson added a comment - +1 this looks sensible to me. I'll commit tonight or tomorrow.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12516257/3.%5B45%5D-0001-notify-on-event-state-not-current-state.patch
        against trunk revision 1294000.

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +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 (version 1.3.9) 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: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/967//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/967//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/967//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/12516257/3.%5B45%5D-0001-notify-on-event-state-not-current-state.patch against trunk revision 1294000. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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 (version 1.3.9) 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: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/967//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/967//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/967//console This message is automatically generated.
        Hide
        Mike Lundy added a comment -

        Recut patches to use -p0 instead of -p1.

        Show
        Mike Lundy added a comment - Recut patches to use -p0 instead of -p1.
        Hide
        Mike Lundy added a comment -

        Note that the patch is cut against 3.3.4, not trunk. (Jenkins ran it against trunk)

        Show
        Mike Lundy added a comment - Note that the patch is cut against 3.3.4, not trunk. (Jenkins ran it against trunk)
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12515480/0001-notify-on-event-state-not-current-state.patch
        against trunk revision 1244776.

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/958//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/12515480/0001-notify-on-event-state-not-current-state.patch against trunk revision 1244776. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/958//console This message is automatically generated.

          People

          • Assignee:
            Mike Lundy
            Reporter:
            Mike Lundy
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development