[
https://issues.apache.org/jira/browse/THRIFT-3218?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14721618#comment-14721618
]
Randy Abernethy commented on THRIFT-3218:
-----------------------------------------
Thanks for the bug report. To fix this we need a few things:
1. A test that fails with the current code
2. A patch that makes the new test pass without causing any of the existing
tests to fail
More here: https://thrift.apache.org/docs/HowToContribute
Also there is a problem with the proposed solution. A scan of the uses of the
seqId2Service map shows the multiplexed_protocol.js source saving service
references in the seqId2Service map with each message write, failing to remove
them will cause the map to grow unbounded. If you are not sure how to otherwise
repair the code, providing a test case that fails would be a help for other
contributors who could potentially fix the bug.
{code:JavaScript}
thrift@ubuntu:~/t2/lib/nodejs$ grep -r -n seqId2Service *
lib/thrift/connection.js:37: this.seqId2Service = {};
lib/thrift/connection.js:128: // in seqId2Service. If the SeqId is
found in the hash we need to
lib/thrift/connection.js:139: var service_name =
self.seqId2Service[header.rseqid];
lib/thrift/connection.js:142: delete self.seqId2Service[header.rseqid];
lib/thrift/http_connection.js:107: this.seqId2Service = {};
lib/thrift/http_connection.js:117: // in seqId2Service. If the SeqId is
found in the hash we need to
lib/thrift/http_connection.js:128: var service_name =
self.seqId2Service[header.rseqid];
lib/thrift/http_connection.js:131: delete
self.seqId2Service[header.rseqid];
lib/thrift/ws_connection.js:95: this.seqId2Service = {};
lib/thrift/ws_connection.js:141: // in seqId2Service. If the SeqId is
found in the hash we need to
lib/thrift/ws_connection.js:152: var service_name =
this.seqId2Service[header.rseqid];
lib/thrift/ws_connection.js:155: delete
this.seqId2Service[header.rseqid];
lib/thrift/xhr_connection.js:70: this.seqId2Service = {};
lib/thrift/xhr_connection.js:151: // in seqId2Service. If the SeqId is
found in the hash we need to
lib/thrift/xhr_connection.js:162: var service_name =
this.seqId2Service[header.rseqid];
lib/thrift/xhr_connection.js:165: delete
this.seqId2Service[header.rseqid];
lib/thrift/multiplexed_protocol.js:34: connection.seqId2Service[seqid] =
serviceName;
{code}
> Large responses when using a MultiplexedProcessor fail to find client after
> first read.
> ---------------------------------------------------------------------------------------
>
> Key: THRIFT-3218
> URL: https://issues.apache.org/jira/browse/THRIFT-3218
> Project: Thrift
> Issue Type: Bug
> Components: Node.js - Library
> Affects Versions: 0.9.2
> Reporter: Bradley Holbrook
> Assignee: Randy Abernethy
> Priority: Blocker
> Labels: easyfix, javascript
>
> Receiving a large response using a multiplexed processor fails to complete
> the read because the service is prematurely deleted in the connection data
> listener callback. Below shows the problem code and the solution deployed to
> solve it.
> {code:javascript|title=Original connection.js client creation}
> while (true) {
> var header = message.readMessageBegin();
> var dummy_seqid = header.rseqid * -1;
> var client = self.client;
> //The Multiplexed Protocol stores a hash of seqid to service names
> // in seqId2Service. If the SeqId is found in the hash we need to
> // lookup the appropriate client for this call.
> // The connection.client object is a single client object when not
> // multiplexing, when using multiplexing it is a service name keyed
> // hash of client objects.
> //NOTE: The 2 way interdependencies between protocols, transports,
> // connections and clients in the Node.js implementation are
> irregular
> // and make the implementation difficult to extend and maintain. We
> // should bring this stuff inline with typical thrift I/O stack
> // operation soon.
> // --ra
> var service_name = self.seqId2Service[header.rseqid];
> if (service_name) {
> client = self.client[service_name];
> delete self.seqId2Service[header.rseqid];
> }
> // ...
> }
> {code}
> {code:javascript|title=Working connection.js client creation}
> while (true) {
> var header = message.readMessageBegin();
> var dummy_seqid = header.rseqid * -1;
> var client = self.client;
> //The Multiplexed Protocol stores a hash of seqid to service names
> // in seqId2Service. If the SeqId is found in the hash we need to
> // lookup the appropriate client for this call.
> // The connection.client object is a single client object when not
> // multiplexing, when using multiplexing it is a service name keyed
> // hash of client objects.
> //NOTE: The 2 way interdependencies between protocols, transports,
> // connections and clients in the Node.js implementation are
> irregular
> // and make the implementation difficult to extend and maintain. We
> // should bring this stuff inline with typical thrift I/O stack
> // operation soon.
> // --ra
> var service_name = self.seqId2Service[header.rseqid];
> if (service_name) {
> client = self.client[service_name];
> }
> // ...
> }
> if(self.seqId2Service[header.rseqid]) {
> delete self.seqId2Service[header.rseqid];
> }
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
