Thrift
  1. Thrift
  2. THRIFT-739

TCompactProtocol isn't suitable for reuse in partialDeserialize

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.3
    • Fix Version/s: 0.3
    • Component/s: Java - Library
    • Labels:
      None

      Description

      Took me a bit to track this one down. Recently, I noticed that one of our applications that uses TDeserializer, TCompactProtocol, and the partialDeserialize method had a very large number of deserialization errors popping up. It turns out that when I implemented direct buffer access in a few spots, I also improved TDeserializer to reuse a single transport and protocol. However, once the partialDeserialize method gets what it's looking for, it returns without consuming the rest of the buffer, leaving a bunch of dangling field id deltas on its internal stack. This causes subsequent partialDeserialization attempts to fail. Once I got one failure, I'd get a new TDeserializer, so it looks like about 50% of my deserializations were failing.

      There seems to be two separate possible solutions for this problem. The first is to add a "reset" method to TProtocol that does nothing in most protocols, but stateful protocols implement so that they can clear themselves out. Then, TDeserializer calls this all the time. The other alternative is to add a "isReusable" or "isStateful" method to TProtocol, and just don't reuse those Protocols that are inappropriate. Personally, I prefer the former solution, but I'd love to hear others' thoughts.

      1. thrift-739.patch
        11 kB
        Bryan Duxbury

        Activity

        Hide
        Bryan Duxbury added a comment -

        I just committed this.

        Show
        Bryan Duxbury added a comment - I just committed this.
        Hide
        Bryan Duxbury added a comment -

        This patch implements the reset() strategy. It also makes sure that partialDeserialize throws TExceptions only, which makes it much easier to catch stuff coming out. Finally, it puts a protocol_.reset() in a finally block in all of the deserialize methods to make sure that stateful protocols will get cleaned up appropriately.

        Show
        Bryan Duxbury added a comment - This patch implements the reset() strategy. It also makes sure that partialDeserialize throws TExceptions only, which makes it much easier to catch stuff coming out. Finally, it puts a protocol_.reset() in a finally block in all of the deserialize methods to make sure that stateful protocols will get cleaned up appropriately.

          People

          • Assignee:
            Bryan Duxbury
            Reporter:
            Bryan Duxbury
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development