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.

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: change default port #7212

Merged
merged 1 commit into from
Jun 9, 2016
Merged

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?

@@ -240,7 +240,7 @@ class V8NodeInspector : public blink::V8Inspector {
bool running_nested_loop_;
};

Agent::Agent(Environment* env) : port_(9229),
Agent::Agent(Environment* env) : port_(0),
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