Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-739

TCompactProtocol isn't suitable for reuse in partialDeserialize

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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.

        Attachments

        1. thrift-739.patch
          11 kB
          Bryan Duxbury

          Activity

            People

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

              Dates

              • Created:
                Updated:
                Resolved: