FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

inspector: change default port by ofrobots · Pull Request #7212 · nodejs/node · GitHub

/ node Public

inspector: change default port - #7212

Merged
ofrobots merged 1 commit into
nodejs:masterfrom
ofrobots:inspector-port
Jun 9, 2016
ConversationCommits (1)ChecksFiles changed
Merged

inspector: change default port#7212
ofrobots merged 1 commit into
nodejs:masterfrom
ofrobots:inspector-port

Conversation

ofrobots commented Jun 8, 2016

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

inspector

Description of change

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.

ofrobots added the inspector Issues and PRs related to the V8 inspector protocol label Jun 8, 2016
nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 8, 2016

bnoordhuis commented Jun 8, 2016

Copy link
Copy Markdown
Member

LGTM

1 similar comment

cjihrig commented Jun 8, 2016

Copy link
Copy Markdown
Contributor

LGTM

cjihrig mentioned this pull request Jun 8, 2016
Closed

ofrobots commented Jun 8, 2016

Copy link
Copy Markdown
Contributor Author

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

MylesBorins commented Jun 8, 2016

Copy link
Copy Markdown
Contributor

@ofrobots just making sure I am reading this change correctly individuals will now be able to pass a custom port to the inspector with --debug-port?

MylesBorins reviewed Jun 8, 2016
View reviewed changes
Comment thread src/inspector_agent.cc Outdated

MylesBorins Jun 8, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

So before the instance is started it will have a port of 0?

joshgav Jun 8, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

from @Trott: #7206, does this apply here?

ofrobots Jun 8, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The port value is actually provided at the time Agent::Start is called. This happened to be the default initialization of the field and the value of 9229 was never getting used. This clean it up to be zero-initialized instead.

joshgav commented Jun 8, 2016

Copy link
Copy Markdown
Contributor

FYI 9229 is not reserved.

As well as palindromic. :)

ofrobots commented Jun 8, 2016

Copy link
Copy Markdown
Contributor Author

@thealphanerd

@ofrobots just making sure I am reading this change correctly individuals will now be able to pass a custom port to the inspector with --debug-port?

Users can still provide a custom port with --inspect=9222. --debug-port=9222 also works, as long as --inspect is also provided.

ofrobots commented Jun 9, 2016

Copy link
Copy Markdown
Contributor Author

The issues on ARM in the CI look like infrastructure issues. Will land this soon.

inspector: change default port
a766ebf 
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.

PR-URL: nodejs#7212
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
ofrobots merged commit a766ebf into nodejs:master Jun 9, 2016
MylesBorins added this to the 7.0.0 milestone Jun 14, 2016
MylesBorins mentioned this pull request Jun 14, 2016
Closed
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
inspector: change default port
ec90a7a 
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.

PR-URL: #7212
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Fishrock123 mentioned this pull request Jul 5, 2016
Merged
MylesBorins mentioned this pull request Jul 11, 2016
Closed
ofrobots mentioned this pull request Sep 15, 2016
Merged
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

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

7.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL