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

inspector: restore 9229 as a default port #8550

Closed
wants to merge 1 commit into from
Closed

inspector: restore 9229 as a default port #8550

wants to merge 1 commit into from

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Sep 15, 2016

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

Some tools are now relying on 9229 to be node.js "inspector" port (I see
Chrome extensions, some online blog posts, etc.) Also, having same
default port values for old and new protocols may lead to some
confusion, e.g. when tools are trying to autodiscover debuggable Node
instances.

CC: @ofrobots, @cjihrig, @bnoordhuis

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 15, 2016
@mscdex mscdex added the inspector Issues and PRs related to the V8 inspector protocol label Sep 15, 2016
ofrobots
ofrobots reviewed Sep 15, 2016
View reviewed changes
Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

nit: can you keep the commit abstract to max 50 chars?

Otherwise LGTM, but let's get @cjihrig to signoff as well and approve.

@ofrobots ofrobots mentioned this pull request Sep 15, 2016
Merged
2 tasks
inspector: restore 9229 as a default port
6646db4 
Some tools are now relying on 9229 to be node.js "inspector" port (I see
Chrome extensions, some online blog posts, etc.) Also, having same
default port values for old and new protocols may lead to some
confusion, e.g. when tools are trying to autodiscover debuggable Node
instances.
@eugeneo eugeneo changed the title inspector: restore 9229 as a default inspector port inspector: restore 9229 as a default port Sep 15, 2016
@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 15, 2016

I've updated the commit message.

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 15, 2016

Failure seems to be coming from "git gc" command execution.

@ofrobots
Copy link
Contributor

New CI: https://ci.nodejs.org/job/node-test-pull-request/4067/

@bnoordhuis
Copy link
Member

Is there a reason you didn't simply revert 9f1f7e2? If yes, can you at least mention in the commit log that you are rolling back that commit?

@cjihrig
Copy link
Contributor

cjihrig commented Sep 16, 2016

Sorry, I'm going to opt out of signing off on this as I don't agree with the change.

@ofrobots
Copy link
Contributor

#8201 was fixed by 9f1f7e2. This change preserves the fix while bringing back the behavior that the old and new protocols run on different ports.

@ofrobots
Copy link
Contributor

@cjihrig: can you elaborate on your concerns? I think debuggers have a valid use-case to require different protocols to run on different ports when trying to attach to existing node processes, or to auto-discover debuggable node processes.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 16, 2016

@ofrobots I didn't realize that this would preserve the fix for #8201 (TBH, I hadn't reviewed these changes at that point, assuming it was essentially a revert). That makes me OK with it.

That said, I'm still worried if debuggers are relying on the port number to determine which protocol is used, as there is nothing stopping users from changing the port. I understand the auto-discovery point, as I'm one of the authors of the aforementioned Chrome extensions.

@joshgav
Copy link
Contributor

joshgav commented Sep 16, 2016

/cc @roblourens @nodejs/diagnostics

@ofrobots
Copy link
Contributor

@cjihrig

That makes me OK with it.

Can you review and sign-off?

cjihrig
cjihrig approved these changes Sep 19, 2016
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM

@ofrobots
Copy link
Contributor

Thanks! Landed as 626a07d.

@ofrobots ofrobots closed this Sep 19, 2016
ofrobots pushed a commit that referenced this pull request Sep 19, 2016
@ofrobots
inspector: restore 9229 as a default port
626a07d 
Some tools are now relying on 9229 to be node.js "inspector" port (I
see Chrome extensions, some online blog posts, etc.) Also, having same
default port values for old and new protocols may lead to some
confusion, e.g. when tools are trying to autodiscover debuggable Node
instances.

This is a partial revert of 9f1f7e2. This commit preserves the fix for
issue #8201 bringing back the behavior that the old and new protocols
run on different ports.run on different ports.

PR-URL: #8550
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
@eugeneo eugeneo deleted the default-ports branch September 20, 2016 16:56
@cjihrig cjihrig mentioned this pull request Oct 6, 2016
Merged
3 tasks
cjihrig added a commit to cjihrig/node that referenced this pull request Oct 10, 2016
@cjihrig
test: add cluster inspector debug port test
2aa2f85 
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 pushed a commit that referenced this pull request Oct 10, 2016
@cjihrig @jasnell
test: add cluster inspector debug port test
96faba6 
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 mentioned this pull request Jun 15, 2017
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ofrobots ofrobots ofrobots left review comments

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@eugeneo @ofrobots @bnoordhuis @cjihrig @joshgav @mscdex @MylesBorins @nodejs-github-bot