[ https://issues.apache.org/jira/browse/THRIFT-2205?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13867126#comment-13867126 ]
Randy Abernethy edited comment on THRIFT-2205 at 1/9/14 10:11 PM: ------------------------------------------------------------------ Hi Pierre, Yeah, I made some notes about this issue in there somewhere. The commit/rollback idiom that was in place before did not work. The Borrow/Consume idiom is used in C++ and other languages to provide efficient transport buffer access to protocols. I started down that road here. The TBufferedTransport implementation is good I think. The TFramedTransport implementation is not baked. The problem with inBuf.length is that it will return the buffer size (like 8K or whatever) rather than the size of the data. I think the actual writeIndex should be something like FrameSize + readIndex. The elephant in the room here is that we need a TJSONProtocol over TFramedTransport test (in addition to your one bracket string test in the generic driver). With this patch I think the status is: TBinaryProtocol/TFramedTransport (Test: sever.js - client.js) works TJSONProtocol/TBufferedTransport (Test: sever_json.js - client_json.js) works TBinaryProtocol/TBufferedTransport (Test: server_bin.js - client_bin.js) works (just uploaded these two in patch 0005) TJSONProtocol/TBufferedTransport (Test: sever_http.js - test.html) the only stack choice for Browser to Node, works TJSONProtocol/TFramedTransport (Test: server_json_frame.js - client_json_frame.js) has the borrow problem you identified -Randy was (Author: codesf): Hi Pierre, Yeah, I made some notes about this issue in there somewhere. The commit/rollback idiom that was in place before did not work. The Borrow/Consume idiom is used in C++ and other languages to provide efficient transport buffer access to protocols. I started down that road here. The TBufferedTransport implementation is good I think. The TFramedTransport implementation is not baked. The problem with inBuf.length is that it will return the buffer size (like 8K or whatever) rather than the size of the data. I think the actual writeIndex should be something like FrameSize + readIndex. The elephant in the room here is that we need a TJSONProtocol over TFramedTransport test (in addition to your one bracket string test in the generic driver). With this patch I think the status is: TBinaryProtocol/TFramedTransport (Test: sever.js - client.js) works TJSONProtocol/TBufferedTransport (Test: sever_json.js - client_json.js) works TBinaryProtocol/TBufferedTransport (Test: server_bin.js - client_bin.js) works (just uploaded these two in patch 0005) TJSONProtocol/TBufferedTransport (Test: sever_http.js - test.html) the only stack choice for Browser to Node, works TJSONProtocol/TFramedTransport (Test: none) has the borrow problem you identified -Randy > Node.js Test Server to support test.js JavaScript Browser test and sundry > fixes > ------------------------------------------------------------------------------- > > Key: THRIFT-2205 > URL: https://issues.apache.org/jira/browse/THRIFT-2205 > Project: Thrift > Issue Type: Improvement > Components: JavaScript - Library, Node.js - Library > Affects Versions: 1.0 > Environment: All > Reporter: Randy Abernethy > Assignee: Randy Abernethy > Priority: Minor > Labels: node, nodejs > Attachments: 0001-node-test-update-with-JSON-Buf-fixes.patch, > 0002-node-client-server-test-update.patch, > 0003-node-throwed-error-should-be-instanciated.patch, > 0004-node-fix-TJSONProtocol-parser.patch, > 0005-node-tests-for-json-frame-and-bin-buf.patch > > > Adds lib/nodejs/test/testsvr.js > This server depends on ThriftTest[.js] and runs clean with test.js in the > browser. > Also in this patch: > Repairs some shortfall in the Node JSON Protocol and transport. Fixes > overflow on Javascript I64 tests. Improves static_server header output. -- This message was sent by Atlassian JIRA (v6.1.5#6160)