thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (THRIFT-4564) TBufferedTransport can leave corrupt data in the buffer
Date Sun, 17 Jun 2018 15:55:00 GMT

    [ https://issues.apache.org/jira/browse/THRIFT-4564?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16515115#comment-16515115
] 

ASF GitHub Bot commented on THRIFT-4564:
----------------------------------------

bforbis commented on issue #1558: THRIFT-4564: Reset buffered transport on serialization errors
URL: https://github.com/apache/thrift/pull/1558#issuecomment-397888034
 
 
   I don't think we have a danger with this. The underlying net socket will not get reset,
just the wrapping buffered transport. Calling `reset()` on the buffered_transport instance
that is responsible for writing will only throw out its internal outBuffers in this case,
which will only have up to 1 message at a time before flushing it to the net socket (flush
is called at the end of send_Foo RPC call in the generated service module). Since this try/catch
is catching serialization errors to the outBuffers, the only thing that would ever be thrown
out is this corrupt half-message in the transports outBuffer, which we won't need anymore
since serialization has failed.
   
   Node will only serialize and send one message at a time, since constructing the message
into the outbuffer and flushing to the socket is done synchronously. Reading is done by an
entirely different instance of buffered_transport, so resetting the transport responsible
for writing will have no affect on receiving messages.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


> 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 - Compiler, 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)

Mime
View raw message