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

inspector: listen on process.debugPort #8386

Merged
merged 1 commit into from Sep 14, 2016

Conversation

Projects
None yet
8 participants
@cjihrig
Contributor

cjihrig commented Sep 2, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector

Description of change

This commit consolidates the debugging port used by the inspector and Node's legacy debugger. This eliminates any chance of the two debuggers working side by side, but the inspector is currently an unofficial feature, and the legacy debugger should eventually be removed in favor of it.

Fixes: #8201

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 2, 2016

LGTM if CI is green.

@addaleax

This comment has been minimized.

Member

addaleax commented Sep 5, 2016

@cjihrig this needs a rebase :)

@cjihrig cjihrig force-pushed the cjihrig:debug-port branch Sep 9, 2016

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Sep 9, 2016

@addaleax rebased. Sorry for the delay. PTAL

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Sep 9, 2016

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Sep 10, 2016

CI failures look unrelated. Since this is semver major, can another CTC member take a look? Perhaps @ofrobots?

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Sep 12, 2016

ping @nodejs/ctc, please.

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Sep 13, 2016

LGTM

1 similar comment
@indutny

This comment has been minimized.

Member

indutny commented Sep 13, 2016

LGTM

inspector: listen on process.debugPort
This commit consolidates the debugging port used by the
inspector and Node's legacy debugger.

Fixes: #8201
PR-URL: #8386
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

@cjihrig cjihrig force-pushed the cjihrig:debug-port branch to 9f1f7e2 Sep 14, 2016

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Sep 14, 2016

@cjihrig cjihrig merged commit 9f1f7e2 into nodejs:master Sep 14, 2016

9 checks passed

test/aix all tests passed
Details
test/arm all tests passed
Details
test/arm-fanned all tests passed
Details
test/freebsd all tests passed
Details
test/linux all tests passed
Details
test/linux-fips all tests passed
Details
test/osx all tests passed
Details
test/smartos all tests passed
Details
test/windows-fanned all tests passed
Details

@cjihrig cjihrig deleted the cjihrig:debug-port branch Sep 14, 2016

@ofrobots

This comment has been minimized.

Contributor

ofrobots commented Sep 15, 2016

Sorry that I missed the ping on this. The use of different ports between inspector and the old debug protocol was quite intentional due to (very valid) concerns raised by the Debugger implementers: See original PR: #7212.

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Sep 15, 2016

Was this it?

We should use a different default port number for the new debug
protocol. This makes it easier for debuggers to guess which protocol
they are expected to use to talk to a node process with a debug
server.

Given that you can set the port manually, wouldn't debuggers have to be able to determine which protocol was being used with 100% certainty?

@ofrobots

This comment has been minimized.

Contributor

ofrobots commented Sep 15, 2016

The issue is that some debuggers support both the old protocol and new, and when a user requests that the debugger attach to a running process at port 5858, the debugger has no way of knowing with protocol to use. I think this was originally requested by the VSCode folks, but I can't seem to find the discussion (/cc @joshgav, @nojvek).

@joshgav

This comment has been minimized.

Member

joshgav commented Sep 15, 2016

Discussion on side-by-side ongoing at #8464, would appreciate input from you all there.

@joshgav

This comment has been minimized.

Member

joshgav commented Sep 15, 2016

@ofrobots

This comment has been minimized.

Contributor

ofrobots commented Sep 15, 2016

@cjihrig cjihrig referenced this pull request Oct 6, 2016

Merged

test: add cluster inspector debug port test #8958

3 of 3 tasks complete

cjihrig added a commit to cjihrig/node-1 that referenced this pull request Oct 10, 2016

test: add cluster inspector debug port test
This commit adds a test for the debug port value in cluster
workers using the inspector debugger.

Refs: nodejs#8201
Refs: nodejs#8386
Refs: nodejs#8550
PR-URL: nodejs#8958
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

jasnell added a commit that referenced this pull request Oct 10, 2016

test: add cluster inspector debug port test
This commit adds a test for the debug port value in cluster
workers using the inspector debugger.

Refs: #8201
Refs: #8386
Refs: #8550
PR-URL: #8958
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment