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

TBufferedTransport can leave corrupt data in the buffer

    XMLWordPrintableJSON

    Details

      Description

      I ran into the following issue on Thrift 0.10.0 using Buffered Transport and Binary Protocol

      IDL File:

      struct Foo {
      1: string name
      }
      
      service SocketTestService {
      i16 countFoos(1: list<Foo> foos)
      }
      

      Client Call:

      async function main() {
      numFoos = await client.countFoos([
      new thriftTypes.Foo(),
      null,
      new thriftTypes.Foo(),
      ]);
      }
      

      The issue I ran into was a client-side thrown exception:

      TypeError: Cannot read property 'write' of null
      at SocketTestService_countFoos_args.write (/Users/bforbis/git/bforbis/thrift-socket-test/gen10/SocketTestService.js:81:15)
      at exports.Client.SocketTestServiceClient.send_countFoos (/Users/bforbis/git/bforbis/thrift-socket-test/gen10/SocketTestService.js:178:8)
      at exports.Client.SocketTestServiceClient.countFoos (/Users/bforbis/git/bforbis/thrift-socket-test/gen10/SocketTestService.js:165:10)
      at main (/Users/bforbis/git/bforbis/thrift-socket-test/client.js:110:28)
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:188:7)
      

      The cause of the issue is because I pass in null as a parameter to my Foo list, which does not have a write() method to serialize itself when writing to the buffer. That's okay on its own, but what ends up happening here is we have half of a message still written to our buffered transport, meaning that a subsequent RPC call on that same transport will fail to be processed on the server side.

      Error: Invalid type: -128
      at TBinaryProtocol.skip (/Users/bforbis/git/bforbis/thrift-socket-test/node_modules/thrift/lib/nodejs/lib/thrift/binary_protocol.js:364:13)
      at module.exports.Foo.Foo.read (/Users/bforbis/git/bforbis/thrift-socket-test/gen11/socket-test_types.js:47:15)
      at SocketTestService_countFoos_args.read (/Users/bforbis/git/bforbis/thrift-socket-test/gen11/SocketTestService.js:51:17)
      at exports.Processor.SocketTestServiceProcessor.process_countFoos (/Users/bforbis/git/bforbis/thrift-socket-test/gen11/SocketTestService.js:224:8)
      at exports.Processor.SocketTestServiceProcessor.process (/Users/bforbis/git/bforbis/thrift-socket-test/gen11/SocketTestService.js:210:39)
      at /Users/bforbis/git/bforbis/thrift-socket-test/node_modules/thrift/lib/nodejs/lib/thrift/server.js:55:21
      at Socket.<anonymous> (/Users/bforbis/git/bforbis/thrift-socket-test/node_modules/thrift/lib/nodejs/lib/thrift/buffered_transport.js:48:5)
      at emitOne (events.js:121:20)
      at Socket.emit (events.js:211:7)
      at addChunk (_stream_readable.js:263:12)
      

      Proposal: We should allow applications to have global persistent socket connections that don't corrupt their buffer if there is a serialization error when doing an RPC all on the client. Calls to send_${RPC_NAME} should be wrapped in a try / catch in case there are client side errors in the processing code. In the case that an exception is thrown, the buffer in buffered transport should be emptied rather than leaving it full.

      Side Note: The cause of the client-side exception has been fixed in Thrift 0.11.0 in this Pull Request, which casts undef to a new struct. But I fear that this issue where hanging data on the buffered transport isn't cleaned up could come up again.

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                jking3 James E. King III
                Reporter:
                bforbis Brian Forbis
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: