ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-794

Callbacks are not invoked when the client is closed

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 3.3.1
    • Fix Version/s: 3.3.2, 3.4.0
    • Component/s: java client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      I noticed that ZooKeeper has different behaviors when calling synchronous or asynchronous actions on a closed ZooKeeper client.
      Actually a synchronous call will throw a "session expired" exception while an asynchronous call will do nothing. No exception, no callback invocation.

      Actually, even if the EventThread receives the Packet with the session expired err code, the packet is never processed since the thread has been killed by the ventOfDeath. So the call back is not invoked.

      1. ZOOKEEPER-794.txt
        13 kB
        Alexis Midon
      2. ZOOKEEPER-794.patch.txt
        13 kB
        Alexis Midon
      3. ZOOKEEPER-794_5.patch.txt
        16 kB
        Alexis Midon
      4. ZOOKEEPER-794_5_br33.patch
        17 kB
        Patrick Hunt
      5. ZOOKEEPER-794_4.patch.txt
        16 kB
        Alexis Midon
      6. ZOOKEEPER-794_3.patch
        5 kB
        Alexis Midon
      7. ZOOKEEPER-794_2.patch
        15 kB
        Benjamin Reed

        Issue Links

          Activity

          Hide
          Alexis Midon added a comment -

          The patch is to not queue the event when the event thread has been killed and process it on the spot.

          Show
          Alexis Midon added a comment - The patch is to not queue the event when the event thread has been killed and process it on the spot.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12448116/ZOOKEEPER-794.patch.txt
          against trunk revision 958096.

          +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 tests are needed for this patch.

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

          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/123/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/12448116/ZOOKEEPER-794.patch.txt against trunk revision 958096. +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 tests are needed for this patch. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/123/console This message is automatically generated.
          Hide
          Alexis Midon added a comment -

          Fix the file paths.

          Show
          Alexis Midon added a comment - Fix the file paths.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12448119/ZOOKEEPER-794.txt
          against trunk revision 958096.

          +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 tests are needed for 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 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-h1.grid.sp2.yahoo.net/124/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/124/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/124/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/12448119/ZOOKEEPER-794.txt against trunk revision 958096. +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 tests are needed for 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 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-h1.grid.sp2.yahoo.net/124/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/124/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/124/console This message is automatically generated.
          Hide
          Mahadev konar added a comment -

          alexis,
          Do you see the problem with c client library as well? Or do you just see the problem with the java client?

          Show
          Mahadev konar added a comment - alexis, Do you see the problem with c client library as well? Or do you just see the problem with the java client?
          Hide
          Benjamin Reed added a comment -

          -1 we need to get a test in. also the fix has a race condition. the boolean flag may changed after it is checked and before the request is queued.

          Show
          Benjamin Reed added a comment - -1 we need to get a test in. also the fix has a race condition. the boolean flag may changed after it is checked and before the request is queued.
          Hide
          Benjamin Reed added a comment -

          i've added a test case and i think i've addressed the race condition. alexis can you check it out. the only change to your code was to make waskilled volatile and move where it was set.

          Show
          Benjamin Reed added a comment - i've added a test case and i think i've addressed the race condition. alexis can you check it out. the only change to your code was to make waskilled volatile and move where it was set.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12449135/ZOOKEEPER-794_2.patch
          against trunk revision 962697.

          +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-h1.grid.sp2.yahoo.net/139/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/139/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/139/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/12449135/ZOOKEEPER-794_2.patch against trunk revision 962697. +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-h1.grid.sp2.yahoo.net/139/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/139/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h1.grid.sp2.yahoo.net/139/console This message is automatically generated.
          Hide
          Alexis Midon added a comment -

          Hi Benjamin,
          thanks a lot for the review.
          One thing I noticed is that setting wasKilled in the method queueEventOfDeath might affect the order in which events are processed. Actually if queueEventOfDeath is invoked while the waitingEvents queue is not empty, any subsequent calls to queuePacket might invoke processEvent for the received Packet before the packets already queued are processed (since wasKilled is true).
          I guess this could happened if there are a lot of events queued in waitingEvents.

          0. Let's assume that, initially, the queue waitingEvents contains N events: Event-0,...,Event-N
          1. queueEventOfDeath is invoked, wasKilled is now true and the queue contains: Event-k,...,Event-N, Event-Death
          2. a new Packet comes in, queuePacket is invoked. Since wasKilled is true, the new event Event-N+1 is processed right away before Event-N might have been processed.

          On the other hand, my initial patch will let Event-N+1 in the queue and it will never get processed since the event of death will break the loop. Which is worse.

          I attached a new patch that aims to fix the ordering issue. The idea is to keep queueing Event-N+i until the event of death has been processed and the queue is empty. I see one downside with this approach: if events keep coming in, the EventThread might not stop since the queue will never get empty.
          Please review.

          Alexis

          Show
          Alexis Midon added a comment - Hi Benjamin, thanks a lot for the review. One thing I noticed is that setting wasKilled in the method queueEventOfDeath might affect the order in which events are processed. Actually if queueEventOfDeath is invoked while the waitingEvents queue is not empty, any subsequent calls to queuePacket might invoke processEvent for the received Packet before the packets already queued are processed (since wasKilled is true). I guess this could happened if there are a lot of events queued in waitingEvents. 0. Let's assume that, initially, the queue waitingEvents contains N events: Event-0,...,Event-N 1. queueEventOfDeath is invoked, wasKilled is now true and the queue contains: Event-k,...,Event-N, Event-Death 2. a new Packet comes in, queuePacket is invoked. Since wasKilled is true, the new event Event-N+1 is processed right away before Event-N might have been processed. On the other hand, my initial patch will let Event-N+1 in the queue and it will never get processed since the event of death will break the loop. Which is worse. I attached a new patch that aims to fix the ordering issue. The idea is to keep queueing Event-N+i until the event of death has been processed and the queue is empty. I see one downside with this approach: if events keep coming in, the EventThread might not stop since the queue will never get empty. Please review. Alexis
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12450827/ZOOKEEPER-794_3.patch
          against trunk revision 980576.

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

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

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

          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/103/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/12450827/ZOOKEEPER-794_3.patch against trunk revision 980576. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/103/console This message is automatically generated.
          Hide
          Benjamin Reed added a comment -

          alexis, i'm missing the problem you are pointing out. is it an issue with the ordering of the callbacks?

          i'm also wondering about your _3 patch. it is much smaller than the others. is it to be applied to trunk, or is it relative to a different patch?

          Show
          Benjamin Reed added a comment - alexis, i'm missing the problem you are pointing out. is it an issue with the ordering of the callbacks? i'm also wondering about your _3 patch. it is much smaller than the others. is it to be applied to trunk, or is it relative to a different patch?
          Hide
          Patrick Hunt added a comment -

          Would be great to get this into 3.3.2, any update?

          Show
          Patrick Hunt added a comment - Would be great to get this into 3.3.2, any update?
          Hide
          Patrick Hunt added a comment -

          cancelling patch - hudson failing.

          Show
          Patrick Hunt added a comment - cancelling patch - hudson failing.
          Hide
          Alexis Midon added a comment -

          Yes, with the first patches, the call back ordering might be different from the event ordering. And this is one of the ZK core garanties right? Although in our case this is happening during the shutdown procedure.

          I'll double check the patch.

          Show
          Alexis Midon added a comment - Yes, with the first patches, the call back ordering might be different from the event ordering. And this is one of the ZK core garanties right? Although in our case this is happening during the shutdown procedure. I'll double check the patch.
          Hide
          Patrick Hunt added a comment -

          What's the status on this? I'd like to include it in upcoming 3.2.2 fix release.

          Show
          Patrick Hunt added a comment - What's the status on this? I'd like to include it in upcoming 3.2.2 fix release.
          Hide
          Alexis Midon added a comment -

          add a new patch that fixes the previous one.

          Show
          Alexis Midon added a comment - add a new patch that fixes the previous one.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12454780/ZOOKEEPER-794_4.patch.txt
          against trunk revision 998200.

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

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

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

          Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/116/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/12454780/ZOOKEEPER-794_4.patch.txt against trunk revision 998200. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/116/console This message is automatically generated.
          Hide
          Patrick Hunt added a comment -

          Canceling patch - failing to apply against svn codebase.

          Can someone update and resubmit?

          Show
          Patrick Hunt added a comment - Canceling patch - failing to apply against svn codebase. Can someone update and resubmit?
          Hide
          Patrick Hunt added a comment -

          I consider this a blocker.

          Show
          Patrick Hunt added a comment - I consider this a blocker.
          Hide
          Alexis Midon added a comment -

          same path as ZOOKEEPER-794_4.patch.txt but strip the path prefix added by Git.

          Show
          Alexis Midon added a comment - same path as ZOOKEEPER-794 _4.patch.txt but strip the path prefix added by Git.
          Hide
          Patrick Hunt added a comment -

          fyi for future - if you name the patch the same name JIRA will take care of it - making the most recent patch active and the older patches of the same name "grey'd out". This helps ensure we have the right patch. Fine for now, but for future reference.

          Thanks!

          Show
          Patrick Hunt added a comment - fyi for future - if you name the patch the same name JIRA will take care of it - making the most recent patch active and the older patches of the same name "grey'd out". This helps ensure we have the right patch. Fine for now, but for future reference. Thanks!
          Hide
          Michi Mutsuzaki added a comment -

          ZOOKEEPER-794_5.patch.txt doesn't compile. I'm getting these errors:

          [javac] branch-3.3/src/java/test/org/apache/zookeeper/test/SessionTest.java:201: cannot find symbol
          [javac] symbol : variable Assert
          [javac] location: class org.apache.zookeeper.test.SessionTest
          [javac] Assert.fail("Should have received a SessionExpiredException");
          [javac] ^
          [javac] branch-3.3/src/java/test/org/apache/zookeeper/test/SessionTest.java:217: cannot find symbol
          [javac] symbol : variable Assert
          [javac] location: class org.apache.zookeeper.test.SessionTest
          [javac] Assert.assertEquals(KeeperException.Code.SESSIONEXPIRED.toString(), cb.toString());
          [javac] ^

          We need to either:

          a. import org.junit.Assert, or
          b. Use fail/assertEquals instead of Assert.fail/Assert.assertEquals.

          --Michi

          Show
          Michi Mutsuzaki added a comment - ZOOKEEPER-794 _5.patch.txt doesn't compile. I'm getting these errors: [javac] branch-3.3/src/java/test/org/apache/zookeeper/test/SessionTest.java:201: cannot find symbol [javac] symbol : variable Assert [javac] location: class org.apache.zookeeper.test.SessionTest [javac] Assert.fail("Should have received a SessionExpiredException"); [javac] ^ [javac] branch-3.3/src/java/test/org/apache/zookeeper/test/SessionTest.java:217: cannot find symbol [javac] symbol : variable Assert [javac] location: class org.apache.zookeeper.test.SessionTest [javac] Assert.assertEquals(KeeperException.Code.SESSIONEXPIRED.toString(), cb.toString()); [javac] ^ We need to either: a. import org.junit.Assert, or b. Use fail/assertEquals instead of Assert.fail/Assert.assertEquals. --Michi
          Hide
          Patrick Hunt added a comment -

          Looks like some recent changes on branch 33 have broken this patch, I'll update the patch and resubmit.

          Show
          Patrick Hunt added a comment - Looks like some recent changes on branch 33 have broken this patch, I'll update the patch and resubmit.
          Hide
          Patrick Hunt added a comment -

          Attaching ZOOKEEPER-794_5_br33 which fixes patching this issue against current branch.

          Michi please give this another try.

          Ben/Flavio/Henry/Mahadev please review for commit asap. This is blocking 3.3.2 release. Thanks all!

          Show
          Patrick Hunt added a comment - Attaching ZOOKEEPER-794 _5_br33 which fixes patching this issue against current branch. Michi please give this another try. Ben/Flavio/Henry/Mahadev please review for commit asap. This is blocking 3.3.2 release. Thanks all!
          Hide
          Michi Mutsuzaki added a comment -

          ZOOKEEPER-794_5_br33.patch worked.

          --Michi

          Show
          Michi Mutsuzaki added a comment - ZOOKEEPER-794 _5_br33.patch worked. --Michi
          Hide
          Patrick Hunt added a comment -

          I tested both trunk and branch patches on my test cluster and it came up green. Ben can you give this a final review and comment if it's ready to commit?

          Show
          Patrick Hunt added a comment - I tested both trunk and branch patches on my test cluster and it came up green. Ben can you give this a final review and comment if it's ready to commit?
          Hide
          Benjamin Reed added a comment -

          +1 thanx for sticking with this one Alexis!

          Show
          Benjamin Reed added a comment - +1 thanx for sticking with this one Alexis!
          Hide
          Alexis Midon added a comment -

          no pb, thanks for your close review and testing.

          Show
          Alexis Midon added a comment - no pb, thanks for your close review and testing.
          Hide
          Patrick Hunt added a comment -

          committed to trunk/branch33, thanks Alexis!

          Show
          Patrick Hunt added a comment - committed to trunk/branch33, thanks Alexis!
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #975 (See https://hudson.apache.org/hudson/job/ZooKeeper-trunk/975/)

          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #975 (See https://hudson.apache.org/hudson/job/ZooKeeper-trunk/975/ )

            People

            • Assignee:
              Alexis Midon
              Reporter:
              Alexis Midon
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development