Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Jun 8, 2016

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 ofrobots added the inspector Issues and PRs related to the V8 inspector protocol label Jun 8, 2016
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 8, 2016
@bnoordhuis
Copy link
Member

LGTM

1 similar comment
@cjihrig
Copy link
Contributor

cjihrig commented Jun 8, 2016

LGTM

@ofrobots
Copy link
Contributor Author

ofrobots commented Jun 8, 2016

@MylesBorins
Copy link
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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

joshgav commented Jun 8, 2016

FYI 9229 is not reserved.

As well as palindromic. :)

@ofrobots
Copy link
Contributor Author

ofrobots commented Jun 8, 2016

@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
Copy link
Contributor Author

ofrobots commented Jun 9, 2016

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

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 ofrobots merged commit a766ebf into nodejs:master Jun 9, 2016
@MylesBorins MylesBorins added this to the 7.0.0 milestone Jun 14, 2016
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
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 Fishrock123 mentioned this pull request Jul 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

7 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.