Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

THRIFT-5710: Stop sharing headers across all instances of transports in nodejs #2805

Merged
merged 1 commit into from
Jun 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/nodejs/lib/thrift/buffered_transport.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ var THeaderTransport = require('./header_transport');
module.exports = TBufferedTransport;

function TBufferedTransport(buffer, callback) {
THeaderTransport.call(this);
this.defaultReadBufferSize = 1024;
this.writeBufferSize = 512; // Soft Limit
this.inBuf = new Buffer(this.defaultReadBufferSize);
Expand All @@ -34,7 +35,7 @@ function TBufferedTransport(buffer, callback) {
this.onFlush = callback;
};

TBufferedTransport.prototype = new THeaderTransport();
Object.setPrototypeOf(TBufferedTransport.prototype, THeaderTransport.prototype);

TBufferedTransport.prototype.reset = function() {
this.inBuf = new Buffer(this.defaultReadBufferSize);
Expand Down
11 changes: 6 additions & 5 deletions lib/nodejs/lib/thrift/framed_transport.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,27 +24,28 @@ var THeaderTransport = require('./header_transport');
module.exports = TFramedTransport;

function TFramedTransport(buffer, callback) {
THeaderTransport.call(this);
this.inBuf = buffer || new Buffer(0);
this.outBuffers = [];
this.outCount = 0;
this.readPos = 0;
this.onFlush = callback;
};

TFramedTransport.prototype = new THeaderTransport();
Object.setPrototypeOf(TFramedTransport.prototype, THeaderTransport.prototype);

TFramedTransport.receiver = function(callback, seqid) {
var residual = [];

return function(data) {
// push received data to residual
for(var i = 0; i < data.length; ++i) {
residual.push(data[i])
}

while (residual.length > 0) {
while (residual.length > 0) {
if (residual.length < 4) {
// Not enough bytes to continue, save and resume on next packet
// Not enough bytes to continue, save and resume on next packet
return;
}
// get single package sieze
Expand All @@ -57,7 +58,7 @@ TFramedTransport.receiver = function(callback, seqid) {
// splice first 4 bytes
residual.splice(0, 4)
// get package data
var frame = Buffer.from(residual.splice(0, frameSize));
var frame = Buffer.from(residual.splice(0, frameSize));
callback(new TFramedTransport(frame), seqid);
}
};
Expand Down
30 changes: 30 additions & 0 deletions lib/nodejs/test/header.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,19 @@ const cases = {
assert.equals(headers.Trace, "abcde");
assert.end();
},
"Should read different headers from different payload": function(assert) {
const transport = new TFramedTransport();
const buf = Buffer.from(headerPayload);
buf[24] = 115; // Change Parent to Parens
buf[32] = 122; // Change shoobar to shoobaz
transport.inBuf = buf;

const headers = transport.readHeaders();
assert.equals(headers.Parent, undefined);
Copy link
Contributor Author

@ngavalas ngavalas May 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be 100% clear, before this fix, headers would have contained { Parent: "shoobar", Parens: "shoobaz", Trace: "abcde" } because it was reusing the same object as the first test. This is not test-only though and happens in real life.

Similar story for the write test.

assert.equals(headers.Parens, "shoobaz");
assert.equals(headers.Trace, "abcde");
assert.end();
},
"Should read headers when reading message begin": function(assert) {
const transport = new TFramedTransport();
transport.inBuf = Buffer.from(headerPayload);
Expand Down Expand Up @@ -70,6 +83,23 @@ const cases = {
assert.equals(headers.boobooboo, "fooshoopoo");
assert.equals(headers.a, "z");
assert.end();
},
"Separate transports should have separate headers": function(assert) {
const writeTransport = new TFramedTransport();
writeTransport.setProtocolId(THeaderTransport.SubprotocolId.BINARY);
writeTransport.setWriteHeader("foo", "bar");
const headers = writeTransport.getWriteHeaders();

const otherWriteTransport = new TFramedTransport();
otherWriteTransport.setProtocolId(THeaderTransport.SubprotocolId.BINARY);
otherWriteTransport.setWriteHeader("otherfoo", "baz");
const otherHeaders = otherWriteTransport.getWriteHeaders();

assert.equals(headers.foo, "bar");
assert.equals(headers.otherfoo, undefined);
assert.equals(otherHeaders.foo, undefined);
assert.equals(otherHeaders.otherfoo, "baz");
assert.end();
}
};

Expand Down