Skip to content
This repository has been archived by the owner on Feb 1, 2022. It is now read-only.

Support for debugging a pid #37

Merged
merged 1 commit into from
Apr 4, 2017
Merged

Support for debugging a pid #37

merged 1 commit into from
Apr 4, 2017

Conversation

jkrems
Copy link
Collaborator

@jkrems jkrems commented Mar 16, 2017

Tested against nodejs/node#11431.

On versions of node that don't use the new protocol by default, this test is skipped (and the feature will most likely not work).

@@ -139,6 +92,56 @@ function portIsFree(host, port, timeout = 2000) {
});
}

function runScript(script, scriptArgs, inspectHost, inspectPort, childPrint) {
return portIsFree(inspectHost, inspectPort)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moving the port check here so it's skipped for remote debugging. Waiting or the the port to be available doesn't make sense for node inspect <host>:<port>/node inspect -p <pid>.

@@ -308,6 +307,22 @@ function parseArgv([target, ...args]) {
port = parseInt(portMatch[1], 10);
script = args[0];
scriptArgs = args.slice(1);
} else if (args.length === 1 && /^\d+$/.test(args[0]) && target === '-p') {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pretty much 1:1 copy of the existing _debugger.js code.

@jkrems
Copy link
Collaborator Author

jkrems commented Mar 16, 2017

/cc @nodejs/diagnostics
/cc @eugeneo (FYI)

@jkrems
Copy link
Collaborator Author

jkrems commented Apr 3, 2017

Manually tested rebased version against nodejs/node#11431, still works.

@jkrems
Copy link
Collaborator Author

jkrems commented Apr 4, 2017

@joshgav If you have some time, this is the last bit that still needs a review.

@jkrems jkrems requested a review from joshgav April 4, 2017 15:15
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM insofar I'm able to judge that.

@jkrems
Copy link
Collaborator Author

jkrems commented Apr 4, 2017

Thanks! Passes on latest nightly & 7.x, merging.

@jkrems jkrems merged commit 5341594 into master Apr 4, 2017
@jkrems jkrems deleted the jk-pid branch April 4, 2017 15:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants