[
https://issues.apache.org/jira/browse/THRIFT-4564?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16519876#comment-16519876
]
ASF GitHub Bot commented on THRIFT-4564:
----------------------------------------
jeking3 closed pull request #1558: THRIFT-4564: Reset buffered transport on
serialization errors
URL: https://github.com/apache/thrift/pull/1558
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/compiler/cpp/src/thrift/generate/t_js_generator.cc
b/compiler/cpp/src/thrift/generate/t_js_generator.cc
index 6f73c1c352..512fe3c1ff 100644
--- a/compiler/cpp/src/thrift/generate/t_js_generator.cc
+++ b/compiler/cpp/src/thrift/generate/t_js_generator.cc
@@ -1529,16 +1529,7 @@ void t_js_generator::generate_service_client(t_service*
tservice) {
std::string messageType = (*f_iter)->is_oneway() ?
"Thrift.MessageType.ONEWAY"
:
"Thrift.MessageType.CALL";
-
- // Serialize the request header
- if (gen_node_) {
- f_service_ << indent() << outputVar << ".writeMessageBegin('" <<
(*f_iter)->get_name()
- << "', " << messageType << ", this.seqid());" << endl;
- } else {
- f_service_ << indent() << outputVar << ".writeMessageBegin('" <<
(*f_iter)->get_name()
- << "', " << messageType << ", this.seqid);" << endl;
- }
-
+ // Build args
if (fields.size() > 0){
f_service_ << indent() << "var params = {" << endl;
for (fld_iter = fields.begin(); fld_iter != fields.end(); ++fld_iter) {
@@ -1555,6 +1546,20 @@ void t_js_generator::generate_service_client(t_service*
tservice) {
f_service_ << indent() << "var args = new " << argsname << "();" << endl;
}
+
+ // Serialize the request header within try/catch
+ f_service_ << indent() << "try {" << endl;
+ indent_up();
+
+ if (gen_node_) {
+ f_service_ << indent() << outputVar << ".writeMessageBegin('" <<
(*f_iter)->get_name()
+ << "', " << messageType << ", this.seqid());" << endl;
+ } else {
+ f_service_ << indent() << outputVar << ".writeMessageBegin('" <<
(*f_iter)->get_name()
+ << "', " << messageType << ", this.seqid);" << endl;
+ }
+
+
// Write to the stream
f_service_ << indent() << "args.write(" << outputVar << ");" << endl <<
indent() << outputVar
<< ".writeMessageEnd();" << endl;
@@ -1605,6 +1610,25 @@ void t_js_generator::generate_service_client(t_service*
tservice) {
}
}
+ indent_down();
+ f_service_ << indent() << "}" << endl;
+
+ // Reset the transport if there was a serialization error
+ f_service_ << indent() << "catch (e) {" << endl;
+ indent_up();
+ if (gen_node_) {
+ f_service_ << indent() << "if (typeof " << outputVar << ".reset ===
'function') {" << endl;
+ f_service_ << indent() << " " << outputVar << ".reset();" << endl;
+ f_service_ << indent() << "}" << endl;
+ } else {
+ f_service_ << indent() << "if (typeof " << outputVar <<
".getTransport().reset === 'function') {" << endl;
+ f_service_ << indent() << " " << outputVar <<
".getTransport().reset();" << endl;
+ f_service_ << indent() << "}" << endl;
+ }
+ f_service_ << indent() << "throw e;" << endl;
+ indent_down();
+ f_service_ << indent() << "}" << endl;
+
indent_down();
f_service_ << "};" << endl;
diff --git a/lib/nodejs/lib/thrift/buffered_transport.js
b/lib/nodejs/lib/thrift/buffered_transport.js
index 13636b574f..a9e006e6cc 100644
--- a/lib/nodejs/lib/thrift/buffered_transport.js
+++ b/lib/nodejs/lib/thrift/buffered_transport.js
@@ -33,6 +33,14 @@ function TBufferedTransport(buffer, callback) {
this.onFlush = callback;
};
+TBufferedTransport.prototype.reset = function() {
+ this.inBuf = new Buffer(this.defaultReadBufferSize);
+ this.readCursor = 0;
+ this.writeCursor = 0;
+ this.outBuffers = [];
+ this.outCount = 0;
+}
+
TBufferedTransport.receiver = function(callback, seqid) {
var reader = new TBufferedTransport();
----------------------------------------------------------------
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:
[email protected]
> 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
> Fix For: 0.12.0
>
>
> 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)
