Skip to content

inspector: restore 9229 as a default port - #8550

Closed
eugeneo wants to merge 1 commit into
nodejs:masterfrom
eugeneo:default-ports
Closed

inspector: restore 9229 as a default port#8550
eugeneo wants to merge 1 commit into
nodejs:masterfrom
eugeneo:default-ports

Conversation

@eugeneo

@eugeneoeugeneo commented Sep 15, 2016

Copy link
Copy Markdown
Contributor
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-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 15, 2016
@mscdexmscdex 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

@ofrobotsofrobots left a comment

Copy link
Copy Markdown
Contributor

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.

@ofrobotsofrobots mentioned this pull request Sep 15, 2016
Merged
2 tasks
Eugene Ostroukhov
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.
@eugeneoeugeneo changed the title inspector: restore 9229 as a default inspector portinspector: restore 9229 as a default portSep 15, 2016
@eugeneo

eugeneo commented Sep 15, 2016

Copy link
Copy Markdown
ContributorAuthor

I've updated the commit message.

@eugeneo

eugeneo commented Sep 15, 2016

Copy link
Copy Markdown
ContributorAuthor

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

@ofrobots

ofrobots commented Sep 16, 2016

Copy link
Copy Markdown
Contributor

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

@bnoordhuis

bnoordhuis commented Sep 16, 2016

Copy link
Copy Markdown
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

cjihrig commented Sep 16, 2016

Copy link
Copy Markdown
Contributor

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

@ofrobots

ofrobots commented Sep 16, 2016

Copy link
Copy Markdown
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

ofrobots commented Sep 16, 2016

Copy link
Copy Markdown
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

cjihrig commented Sep 16, 2016

Copy link
Copy Markdown
Contributor

@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

joshgav commented Sep 16, 2016

Copy link
Copy Markdown
Contributor

/cc @roblourens @nodejs/diagnostics

@ofrobots

ofrobots commented Sep 19, 2016

Copy link
Copy Markdown
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

@cjihrigcjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@ofrobots

ofrobots commented Sep 19, 2016

Copy link
Copy Markdown
Contributor

Thanks! Landed as 626a07d.

ofrobots pushed a commit that referenced this pull request Sep 19, 2016
Eugene Ostroukhov@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
@cjihrigcjihrig 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>
@gibfahngibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjihrigcjihrigcjihrig approved these changes
+1 more reviewer
@ofrobotsofrobotsofrobots left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.inspectorIssues 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