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

TJSONProtocol is unsafe to be used with TDeserializerPool

    XMLWordPrintableJSON

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 0.16.0
    • 0.17.0
    • Go - Library
    • None

    Description

      We just realized that when using TJSONProtocol with TDeserializerPool like this:

      var deserializerPool = thrift.NewTDeserializerPoolSizeFactory(1024, thrift.NewTJSONProtocolFactory())
      

      It only works when it never fails (e.g. it never encounters invalid json blobs). Whenever a failure happens, because of the internal state stack and other states in TJSONProtocol, it will be put back into the pool with the wrong state, and when it's next used it will fail again.

      There are a few approaches we can take to fix it:

      1. The simplest "fix" would be just document in TDeserializerPool and TSerializerPool that TJSONProtocol and TSimpleJSONProtocol are not safe to be used, and maybe provide some examples of how to use them (only pool the TMemoryBuffer and recreate TProtocol every time)
      2. In TJSONProtocol and TSimpleJSONProtocol's [Read|Write][Message|Struct]Begin, implicitly reset the internal state. But I'm not sure how safe that is
      3. Make a breaking change to TProtocol to add a Reset (or maybe ResetState) API to be used by TDeserializer/TSerializer (similar to how they always reset the TMemoryBuffer before doing anything)
      4. Similar to 3 but less breaking, just add a Reset/ResetState to TJSONProtocol and TSimpleJSONProtocol (but not TProtocol), and in TDeserializer/TSerializer do a type assertion and call the Reset API on the protocol if it exists.

      Attachments

        Issue Links

          Activity

            People

              fishywang Yuxuan Wang
              fishywang Yuxuan Wang
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Time Tracking

                  Estimated:
                  Original Estimate - Not Specified
                  Not Specified
                  Remaining:
                  Remaining Estimate - 0h
                  0h
                  Logged:
                  Time Spent - 20m
                  20m