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: improve ESRCH error message #1863

Closed
wants to merge 1 commit into from
+51 −3
Closed

debugger: improve ESRCH error message #1863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 12 additions & 3 deletions lib/_debugger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1639,7 +1639,16 @@ Interface.prototype.trySpawn = function(cb) {
} else if (this.args.length === 3) {
// `node debug -p pid`
if (this.args[1] === '-p' && /^\d+$/.test(this.args[2])) {
process._debugProcess(parseInt(this.args[2], 10));
const pid = parseInt(this.args[2], 10);
try {
process._debugProcess(pid);
} catch (e) {
if (e.code === 'ESRCH') {
Copy link
Member

Choose a reason for hiding this comment

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

I have a question to reviewers.

process.bindings('uv') has some error definitions like ESRCH.
so we can write the following code :

const uv = process.bindings('uv');

if (e.code === uv.errname(uv.UV_ESRCH)) {
  // ...
}

is this better way? if any reviewer has some opinions, please tell me.
if this code is good, we can prevent some typos like ESCRH.

Copy link
Member

Choose a reason for hiding this comment

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

It's alright but I wouldn't write it like that myself, it's more lines of code and harder to grok than a simple string comparison. Typos shouldn't be an issue because there should be regression tests that catch those.

console.error(`Target process: ${pid} doesn't exist.`);
process.exit(1);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's better to update err.message and rethrow? It's better to have a single point of exit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default handle is too general, I don't know how to improve it.

  process.on('uncaughtException', function(e) {
    console.error("There was an internal error in Node's debugger. " +
        'Please report this bug.');
    console.error(e.message);
    console.error(e.stack);
    if (interface_.child) interface_.child.kill();
    process.exit(1);
  });

}
throw e;
}
isRemote = true;
} else {
var match = this.args[1].match(/^--port=(\d+)$/);
Expand Down Expand Up @@ -1704,8 +1713,8 @@ Interface.prototype.trySpawn = function(cb) {
function connectError() {
// If it's failed to connect 10 times then print failed message
if (connectionAttempts >= 10) {
self.stdout.write(' failed, please retry\n');
return;
console.error(' failed, please retry');
process.exit(1);
Copy link
Member

Choose a reason for hiding this comment

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

Is this a debug leftover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It failed to connect target process.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like an arbitrary change. Leaving the return statement in is a bit odd as well.

}
setTimeout(attemptConnect, 500);
}
Expand Down
39 changes: 39 additions & 0 deletions test/debugger/test-debugger-pid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
'use strict';
var common = require('../common');
var assert = require('assert');
var spawn = require('child_process').spawn;

var port = common.PORT + 1337;
var buffer = '';
var expected = [];
var scriptToDebug = common.fixturesDir + '/empty.js';

function fail() {
assert(0); // `--debug-brk script.js` should not quit
}

// connect to debug agent
var interfacer = spawn(process.execPath, ['debug', '-p', '655555']);

console.error(process.execPath, 'debug', '-p', '655555');
interfacer.stdout.setEncoding('utf-8');
interfacer.stderr.setEncoding('utf-8');
var onData = function (data) {
data = (buffer + data).split('\n');
buffer = data.pop();
data.forEach(function(line) {
interfacer.emit('line', line);
});
};
interfacer.stdout.on('data', onData);
interfacer.stderr.on('data', onData);

interfacer.on('line', function(line) {
line = line.replace(/^(debug> *)+/, '');
var expected = 'Target process: 655555 doesn\'t exist.';
assert.ok(expected == line, 'Got unexpected line: ' + line);
});

interfacer.on('exit', function (code, signal) {
assert.ok(code == 1, 'Got unexpected code: ' + code);
});