-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
debug-agent: improvements #1977
Conversation
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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)
cc @indutny (I think?) |
@@ -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\"}"; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Needs test with multiple clients trying to connect, and only one client succeeding. The the third client should be able to connect again after that succeeded one disconnected. |
Otherwise LGTM. Thank you! |
@3y3 ... is this still something you'd like to pursue? |
Closing due to lack of activity and response. |
Closes #858
Partially closes #781