ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-562

c client can flood server with pings if tcp send queue filled

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 3.2.1
    • Fix Version/s: 3.1.2, 3.2.2, 3.3.0
    • Component/s: c client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Hide
      Basically the problem here is that the client gets confused, it tries to send a ping to the server but since the tcp queue is full it's unable to do so. The logic responsible for sending occasional pings based on the timeout gets confused by this, and ends up flooding the server with pings. Eventually this clears up, however it can result in increased load on the server and instability for the effected client.
      Show
      Basically the problem here is that the client gets confused, it tries to send a ping to the server but since the tcp queue is full it's unable to do so. The logic responsible for sending occasional pings based on the timeout gets confused by this, and ends up flooding the server with pings. Eventually this clears up, however it can result in increased load on the server and instability for the effected client.

      Description

      The c client can flood the server with pings if the tcp queue is filled.

      Say the cluster is overloaded and shuts down the recv processing

      a c client can send a ping, but since last_send is only updated on successful pushing of data into the
      socket, if flush_send_queue fails to send any data (send_buffer returns 0) then last_send is not updated
      and zookeeper_interest will again send a ping the next time it is woken - which could be 0 if recv_to is close
      to 0, easily could happen if server is not sending data to the client.

        Activity

        Hide
        Patrick Hunt added a comment -

        send_ping is calling wake_io_thread itself, so this is a particularly bad situation (forces a tight loop)

        solution is to update last_send as last_send_attempt when attempting to send, whether successful or not.

        Show
        Patrick Hunt added a comment - send_ping is calling wake_io_thread itself, so this is a particularly bad situation (forces a tight loop) solution is to update last_send as last_send_attempt when attempting to send, whether successful or not.
        Hide
        Patrick Hunt added a comment -

        Assigning to Ben.

        We should verify that something like this can't happen in the java either. From my looking
        it seems not, but would be good to have addl verification.

        Show
        Patrick Hunt added a comment - Assigning to Ben. We should verify that something like this can't happen in the java either. From my looking it seems not, but would be good to have addl verification.
        Hide
        Benjamin Reed added a comment -

        this patch fixes the problem by only sending a ping if there isn't something already queued. the test checks for clients sending gratuitous pings.

        Show
        Benjamin Reed added a comment - this patch fixes the problem by only sending a ping if there isn't something already queued. the test checks for clients sending gratuitous pings.
        Hide
        Hadoop QA added a comment -

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

        +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/40/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/40/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/40/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/12423282/ZOOKEEPER-562.patch against trunk revision 828216. +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/40/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/40/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/40/console This message is automatically generated.
        Hide
        Patrick Hunt added a comment -

        +1, looks good to me - if we are waiting for a result we don't need to send a ping. great!

        Mahadev, can you take a look at this and commit if no issues found? (both 3.2 branch and trunk)

        Show
        Patrick Hunt added a comment - +1, looks good to me - if we are waiting for a result we don't need to send a ping. great! Mahadev, can you take a look at this and commit if no issues found? (both 3.2 branch and trunk)
        Hide
        Mahadev konar added a comment -

        +1 this looks good.

        Show
        Mahadev konar added a comment - +1 this looks good.
        Hide
        Mahadev konar added a comment -

        I just committed this to 3.2 branch and trunk.

        Show
        Mahadev konar added a comment - I just committed this to 3.2 branch and trunk.
        Hide
        Hudson added a comment -

        Integrated in ZooKeeper-trunk #513 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/513/)
        . c client can flood server with pings if tcp send queue filled. (ben reed via mahadev)

        Show
        Hudson added a comment - Integrated in ZooKeeper-trunk #513 (See http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/513/ ) . c client can flood server with pings if tcp send queue filled. (ben reed via mahadev)

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development