thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Brian Forbis (JIRA)" <j...@apache.org>
Subject [jira] [Created] (THRIFT-4564) TBufferedTransport can leave corrupt data in the buffer
Date Fri, 04 May 2018 16:39:00 GMT
Brian Forbis created THRIFT-4564:
------------------------------------

             Summary: TBufferedTransport can leave corrupt data in the buffer
                 Key: THRIFT-4564
                 URL: https://issues.apache.org/jira/browse/THRIFT-4564
             Project: Thrift
          Issue Type: Bug
          Components: Node.js - Library
    Affects Versions: 0.11.0, 0.10.0
            Reporter: Brian Forbis


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

IDL File:
{code:java}
struct Foo {
1: string name
}

service SocketTestService {
i16 countFoos(1: list<Foo> foos)
}
{code}
Client Call:
{code:java}
async function main() {
numFoos = await client.countFoos([
new thriftTypes.Foo(),
null,
new thriftTypes.Foo(),
]);
}
{code}
The issue I ran into was a client-side thrown exception:
{code:java}
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)
{code}
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.
{code:java}
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)
{code}
*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|https://github.com/apache/thrift/commit/de112fbb0d7f2139ef107211e82e03b574f890d0#diff-a686530169a8a14044fae0f95d54578c],
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.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message