Uploaded image for project: 'Spymemcached Java Client'
  1. Spymemcached Java Client
  2. SPY-89

invalid binary protocol server response during rebalance

    Details

    • Type: Bug
    • Status: Open
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: 2.8.1
    • Fix Version/s: .next
    • Component/s: library
    • Security Level: Public
    • Labels:
      None

      Description

      When investigating an issue, I found that the server would unexpectedly respond with a 0x0a command for optimized get requests. This seems to be in violation of the protocol, and it causes all kinds of havoc for a client library.

      The scenario is spymemcached's internals doing an optimized get. It's reading the response for an operation during a rebalance. It has been observed with both the node being rebalanced into the cluster and with a node leaving the cluster.

      Reading the minimum header, the client library is checking the magic, and then the response command. It's expecting a 0x00, but receiving a 0x0a.

      I poked through the server side code, and 0x0a corresponds to TAP_NOOP. I poked around a little further, and I don't see any situation where we would respond with this 0x0a to a non-TAP client.

      The assertion this is raising is right here:
      https://github.com/dustin/java-memcached-client/blob/master/src/main/java/net/spy/memcached/protocol/binary/OperationImpl.java#L136

      If I force spymemcached to not optimize, the problem goes away.

      I will attach a packet capture.

      1. rebalanceout.out
        6.10 MB
        Matt Ingenthron
      2. rebalanceout-notes.txt
        0.3 kB
        Matt Ingenthron
      No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

        Hide
        daschl Michael Nitschinger added a comment -

        Is this still an issue that needs to be investigated?

        Show
        daschl Michael Nitschinger added a comment - Is this still an issue that needs to be investigated?
        Hide
        ingenthr Matt Ingenthron added a comment -

        Moved this, as I found the protocol was correct, but the assertion was incorrect. The assertion did not account for any setq responses with errors, so this would have been an issue in any environment with optimized sets and responses on setqs.

        Show
        ingenthr Matt Ingenthron added a comment - Moved this, as I found the protocol was correct, but the assertion was incorrect. The assertion did not account for any setq responses with errors, so this would have been an issue in any environment with optimized sets and responses on setqs.
        Hide
        trond Trond Norbye added a comment -

        I moved this bug back to the client for investigation if we did send a noop request or not (this is typically sent after a series of quiet commands in order to ensure that we get a response). Could it be that we don't correctly handle reshuffling of the packets (adding a noop to all of the new queues, and ensure that we don't send multiple of them since we most likely expect only once).

        Show
        trond Trond Norbye added a comment - I moved this bug back to the client for investigation if we did send a noop request or not (this is typically sent after a series of quiet commands in order to ensure that we get a response). Could it be that we don't correctly handle reshuffling of the packets (adding a noop to all of the new queues, and ensure that we don't send multiple of them since we most likely expect only once).
        Hide
        trond Trond Norbye added a comment -

        0x0a == NOOP, not TAP_NOOP...

        Show
        trond Trond Norbye added a comment - 0x0a == NOOP, not TAP_NOOP...
        Hide
        ingenthr Matt Ingenthron added a comment -

        Added packet capture and notes (which are effectively the same as above)

        Show
        ingenthr Matt Ingenthron added a comment - Added packet capture and notes (which are effectively the same as above)

          People

          • Assignee:
            ingenthr Matt Ingenthron
            Reporter:
            ingenthr Matt Ingenthron
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Gerrit Reviews

              There are no open Gerrit changes