[
https://issues.apache.org/jira/browse/THRIFT-4564?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16464117#comment-16464117
]
Brian Forbis commented on THRIFT-4564:
--------------------------------------
This may affect other languages as well, and I also don't know how this behaves
with very large messages. Another thing I noticed is that subsequent RPC
requests using JSONProtocol do NOT have an issue. I am not sure why yet.
> 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.10.0, 0.11.0
> Reporter: Brian Forbis
> Priority: Minor
>
> 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)
