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

debug-agent: improvements #1977

Closed
wants to merge 3 commits into from
Closed

debug-agent: improvements #1977

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
15dcdb8
debug-agent: allow only one client connection
3y3 May 22, 2015
c6fccee
debug-agent: handle client connection errors
3y3 May 22, 2015
550204e
debug-agent: handle `disconnect` message
3y3 May 22, 2015
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
31 changes: 23 additions & 8 deletions lib/_debug_agent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ const util = require('util');
const Buffer = require('buffer').Buffer;
const Transform = require('stream').Transform;

const disconnectRequest = "{\"seq\":1,\"type\":\"request\",\"command\":\"disconnect\"}";
Copy link
Member

Choose a reason for hiding this comment

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

Any point in not using JSON.stringify() here?

Copy link
Author

Choose a reason for hiding this comment

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

This is backported from old debugger implementation, there is no other reasons.


exports.start = function start() {
var agent = new Agent();

Expand Down Expand Up @@ -45,18 +47,21 @@ exports.start = function start() {

function Agent() {
net.Server.call(this, this.onConnection);
this.maxConnections = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are we using this?

Copy link
Author

Choose a reason for hiding this comment

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

This prevent debugger from accepting more than one client. Please, read #858 to understand, why we need to limit clients number.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add a comment about that, to someone unfamiliar with the entire net Server API this will be confusing.


this.first = true;
this.binding = process._debugAPI;

var self = this;
this.binding.onmessage = function(msg) {
self.clients.forEach(function(client) {
client.send({}, msg);
});
if (self.client)
self.client.send({}, msg);
};

this.clients = [];
this.client = null;
this.on('close', function() {
self.client = null;
});
assert(this.binding, 'Debugger agent running without bindings!');
}
util.inherits(Agent, net.Server);
Expand All @@ -65,13 +70,11 @@ Agent.prototype.onConnection = function onConnection(socket) {
var c = new Client(this, socket);

c.start();
this.clients.push(c);
this.client = c;

var self = this;
c.once('close', function() {
var index = self.clients.indexOf(c);
assert(index !== -1);
self.clients.splice(index, 1);
self.client = null;
});
};

Expand All @@ -89,6 +92,7 @@ function Client(agent, socket) {
this.agent = agent;
this.binding = this.agent.binding;
this.socket = socket;
this.disconnected = false;

// Parse incoming data
this.state = 'headers';
Expand All @@ -102,11 +106,17 @@ function Client(agent, socket) {
this.socket.on('close', function() {
self.destroy();
});
this.socket.on('error', function(err) {
// Silently handle socket errors
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are silently ignoring the errors then why do we need this?

Copy link
Author

Choose a reason for hiding this comment

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

Unhandled error will be thrown and kills app.
Unhandled 'error' event...

Copy link
Author

Choose a reason for hiding this comment

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

Also this is a place for discussion. I don't know how we need to handle debugger connection errors (My opinion is - we don't need to do anything on debugger connection errors, because this is debugging of debugger)

});
}
util.inherits(Client, Transform);

Client.prototype.destroy = function destroy(msg) {
this.socket.destroy();

if (!this.disconnected)
this.binding.sendCommand(disconnectRequest);

this.emit('close');
};
Expand Down Expand Up @@ -185,6 +195,11 @@ Client.prototype.onCommand = function onCommand(cmd) {
this.binding.sendCommand(cmd.body);

this.agent.notifyWait();

if (cmd.body == disconnectRequest) {
Copy link
Member

Choose a reason for hiding this comment

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

===

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to JSON.parse the command and check it's type? It could be guarded by RegExp match if you are afraid of hurting performance.

Copy link
Author

Choose a reason for hiding this comment

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

I won't use JSON.parse here because I'm worried about performance.

So, how I can understand you worried about situation, when we receive from user a disconnect request with other order of serialized properties.
But the most bad that can happens in this situation - we'll send two disconnect requests to debugger (one real from user and one other generated by debug-agent script). This behavior also exists in old debugger.

Unless you want to send two disconnect requests to debugger, I will think how to avoid this situation with minimal performance loss.

this.disconnected = true;
this.destroy();
}
};

function Command(headers, body) {
Expand Down