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.

Sign up for GitHub

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

Jump to bottom

debugger: fix debugger failure when passing commands with non-latin symbols #11841

Closed
wants to merge 3 commits into from
Closed

debugger: fix debugger failure when passing commands with non-latin symbols #11841

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0c659ed
debugger: fix debugger failure when passing commands with non-latin c…
Let0s Mar 14, 2017
2ef054d
change Client.buffer type from string to Buffer
Let0s Mar 16, 2017
a3cd3cf
fix
Let0s Mar 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions lib/_debug_agent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ function Client(agent, socket) {
// Parse incoming data
this.state = 'headers';
this.headers = {};
this.buffer = '';
this.buffer = new Buffer(0);
Copy link
Member

Choose a reason for hiding this comment

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

Can you use Buffer.alloc(0) here?

socket.pipe(this);

this.on('data', this.onCommand);
Expand All @@ -117,39 +117,40 @@ Client.prototype.destroy = function destroy(msg) {
Client.prototype._transform = function _transform(data, enc, cb) {
cb();

this.buffer += data;
this.buffer = Buffer.concat([this.buffer, data]);

while (true) {
if (this.state === 'headers') {
// Not enough data
if (!this.buffer.includes('\r\n'))
break;

if (this.buffer.startsWith('\r\n')) {
var bufString = this.buffer.toString('utf8');
if (bufString.startsWith('\r\n')) {
this.buffer = this.buffer.slice(2);
this.state = 'body';
continue;
}

// Match:
// Header-name: header-value\r\n
var match = this.buffer.match(/^([^:\s\r\n]+)\s*:\s*([^\s\r\n]+)\r\n/);
var match = bufString.match(/^([^:\s\r\n]+)\s*:\s*([^\s\r\n]+)\r\n/);
if (!match)
return this.destroy('Expected header, but failed to parse it');

this.headers[match[1].toLowerCase()] = match[2];

this.buffer = this.buffer.slice(match[0].length);
this.buffer = this.buffer.slice(Buffer.byteLength(match[0], 'utf8'));
Copy link
Member

Choose a reason for hiding this comment

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

There will likely be a bit of a performance hit on this given the multiple utf8 conversions involved on chunks of the buffer (the toString above, the byteLength call here). We should benchmark this change to see how much of a hit this causes.

Copy link
Member

Choose a reason for hiding this comment

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

this.buffer could be turned into a string_decoder.StringDecoder. It's more structured than manual buffer->string manipulation and should be more efficient too.

} else {
var len = this.headers['content-length'];
if (len === undefined)
return this.destroy('Expected content-length');

len = len | 0;
if (Buffer.byteLength(this.buffer) < len)
if (Buffer.byteLength(this.buffer, 'utf8') < len)
break;

this.push(new Command(this.headers, this.buffer.slice(0, len)));
this.push(new Command(this.headers, this.buffer.slice(0, len).toString('utf8')));
Copy link
Member

Choose a reason for hiding this comment

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

Long line. Can you run make test, it also lints source files.

this.state = 'headers';
this.buffer = this.buffer.slice(len);
this.headers = {};
Expand Down