Thrift
  1. Thrift
  2. THRIFT-760

Generated client code does not set or check the sequence ID in messages

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.2
    • Fix Version/s: 0.3
    • Component/s: Java - Compiler
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      The sequence ID is never set in the generated client code. It is also never checked. This means that if you continue to use a connection after a socket timeout the returned results can arrive out of sequence. When this happens an exception should be thrown so that incorrect results are never returned.

        Activity

        Hide
        James Grant added a comment -

        Attached a patch for the Java generator that sets and checks the sequence ID

        Show
        James Grant added a comment - Attached a patch for the Java generator that sets and checks the sequence ID
        Hide
        Bryan Duxbury added a comment -

        I think this is generally cool, but won't it break Java's ability to interact with any language that doesn't implement the seqid functionality yet? My impression is that we've left the capability, but not yet implemented it anywhere.

        Show
        Bryan Duxbury added a comment - I think this is generally cool, but won't it break Java's ability to interact with any language that doesn't implement the seqid functionality yet? My impression is that we've left the capability, but not yet implemented it anywhere.
        Hide
        James Grant added a comment -

        This change is only for the client. I have checked the server part of the generator for C++, Python, C#, PHP and Java and they all correctly return the seqid sent in the request.

        This change only checks that value and throws an exception when they do not match.

        The current behavior will either fail with a protocol error or return an incorrect result. Although the patch I supplied only applies to the Java client I think the same change should be made to the other languages.

        Show
        James Grant added a comment - This change is only for the client. I have checked the server part of the generator for C++, Python, C#, PHP and Java and they all correctly return the seqid sent in the request. This change only checks that value and throws an exception when they do not match. The current behavior will either fail with a protocol error or return an incorrect result. Although the patch I supplied only applies to the Java client I think the same change should be made to the other languages.
        Hide
        Marc de Palol added a comment -

        Indeed.

        This bug caused me serious trouble. Under high load the server can respond a message to a wrong client. Then the client thinks that the response is right and continues its workflow with wrong data, instead it should fail and retry the call. Of course this should be handled at application level, but at least there should be a way to know that the data it got is not what's meant to be.

        Show
        Marc de Palol added a comment - Indeed. This bug caused me serious trouble. Under high load the server can respond a message to a wrong client. Then the client thinks that the response is right and continues its workflow with wrong data, instead it should fail and retry the call. Of course this should be handled at application level, but at least there should be a way to know that the data it got is not what's meant to be.
        Hide
        Bryan Duxbury added a comment -

        @Marc - I'm confused as to how the server can send a message back to the wrong client. If that's the case, then this is a very serious issue which seems separate from the sequence ID stuff.

        Show
        Bryan Duxbury added a comment - @Marc - I'm confused as to how the server can send a message back to the wrong client. If that's the case, then this is a very serious issue which seems separate from the sequence ID stuff.
        Hide
        Bryan Duxbury added a comment -

        I just committed this patch. Thanks for the contribution, James!

        Show
        Bryan Duxbury added a comment - I just committed this patch. Thanks for the contribution, James!

          People

          • Assignee:
            James Grant
            Reporter:
            James Grant
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development