inspector: split the HostPort being used and the one parsed from CLI #24772

joyeecheung · 2018-12-01T17:42:07Z

Instead of using a shared pointer of the entire debug option set,
pass the parsed debug option to inspector classes by value because
they are set once the CLI argument parsing is done. Add another shared
pointer to HostPort being used by the inspector server, which is copied
from the one in the debug options initially. The port of the shared
HostPort is 9229 by default and can be specified as 0 initially but
will be set to the actual port of the server once it starts listening.

This makes the shared state clearer and makes it possible to use
require('internal/options') in JS land to query the CLI options
instead of using process._breakFirstLine and other underscored
properties of process since we are now certain that these
values should not be altered once the parsing is done and can be
passed around in copies without locks.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2018-12-01T17:42:09Z

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1764/pipeline

joyeecheung · 2018-12-01T17:43:26Z

cc @addaleax @eugeneo

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

src/node_options.h

src/inspector_agent.h

bnoordhuis · 2018-12-02T15:23:25Z

src/inspector_js_api.cc

@@ -243,12 +243,12 @@ void Open(const FunctionCallbackInfo<Value>& args) {

 if (args.Length() > 0 && args[0]->IsUint32()) {
 uint32_t port = args[0].As<Uint32>()->Value();
- agent->options()->host_port.port = port;
+ agent->host_port()->set_port(static_cast<int>(port));


The cast can overflow. Better to check that args[0]->IsInt32() and that args[0]->Int32Value() > 0.

It was already possible to overflow before since we were assigning uint32_t to int anyways. I think we'd better turn port into a uint16_t (see the TODO in node_options.h) and do the checks whenever we set it, but I'd prefer to do that in another patch, because the port is actually settable from user land so there will be a more obvious behavior change than in this patch. (I am also thinking about using gsl::narrow to do it but we will need to introduce GSL as a dependency first...or maybe we can simply mask that with 0xffff)

bnoordhuis · 2018-12-02T15:27:26Z

src/node_process.cc

- env->options()->debug_options->host_port.port =
-  value->Int32Value(env->context()).FromMaybe(0);
+ int32_t port = value->Int32Value(env->context()).FromMaybe(0);
+ env->inspector_host_port()->set_port(static_cast<int>(port));


Unnecessary cast, right?

I guess it's better to be explicit than implicit? Also I plan to turn this into a uint16_t in the future - I think that's also where things like GSL (see #23821) may be useful

bnoordhuis · 2018-12-02T15:28:31Z

src/inspector_agent.h

+ // This is a copy of the debug options parsed from CI in the Environment.
+ // Do not use the host_port in that, instead manipulate the shared host_port_
+ // pointer which is meant to store the actual host and port of the inspector
+ // server.


It's kind of gnarly to have those fields duplicated. Can that be fixed somehow?

I've tried a few approaches, e.g. splitting the options into two Options subclasses instead of putting them in one DebugOptions, that makes the parsing code even more complicated because we have combined arguments like --inspect-brk=127.0.0.1:0. Also, the HostPort in DebugOptions are actually per-process (because those are parsed from CLI) whereas the shared HostPort are per-Environment (since the inspector agents are per-Environment). Conceptually, they are not really duplicates because the CLI ones are "snapshots" of the parsed arguments which may still be useful themselves (e.g. in tests)

src/node_process.cc

joyeecheung · 2018-12-03T18:46:31Z

Removed the unnecessary per-process lock in DebugPortGetter and DebugPortSetter since they are now per-Environment shared pointers.

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

joyeecheung · 2018-12-03T20:14:47Z

Added a few compile-time switches to make it build when inspector is not enabled (like before, that will result in all the debug options being default..I wonder whether we want to just eliminate them all in the future). CI: https://ci.nodejs.org/job/node-test-pull-request/19164/

addaleax · 2018-12-04T09:50:53Z

@joyeecheung Looks like this might need a rebase?

joyeecheung · 2018-12-04T10:55:30Z

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

Trott · 2018-12-04T15:51:11Z

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19198/ ✔️

Instead of using a shared pointer of the entire debug option set, pass the parsed debug option to inspector classes by value because they are set once the CLI argument parsing is done. Add another shared pointer to HostPort being used by the inspector server, which is copied from the one in the debug options initially. The port of the shared HostPort is 9229 by default and can be specified as 0 initially but will be set to the actual port of the server once it starts listening. This makes the shared state clearer and makes it possible to use `require('internal/options')` in JS land to query the CLI options instead of using `process._breakFirstLine` and other underscored properties of `process` since we are now certain that these values should not be altered once the parsing is done and can be passed around in copies without locks.

joyeecheung · 2018-12-06T22:40:13Z

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

This needs one more approval to land. Can I have some reviews please? @nodejs/v8-inspector @nodejs/process @nodejs/build-files

joyeecheung · 2018-12-08T13:23:23Z

Resume: https://ci.nodejs.org/job/node-test-pull-request/19320/

joyeecheung · 2018-12-08T20:35:22Z

Resume: https://ci.nodejs.org/job/node-test-pull-request/19332/

(Only failure in the previous CI run was #24403 , though)

joyeecheung · 2018-12-08T22:51:18Z

Landed in 61a8963

Instead of using a shared pointer of the entire debug option set, pass the parsed debug option to inspector classes by value because they are set once the CLI argument parsing is done. Add another shared pointer to HostPort being used by the inspector server, which is copied from the one in the debug options initially. The port of the shared HostPort is 9229 by default and can be specified as 0 initially but will be set to the actual port of the server once it starts listening. This makes the shared state clearer and makes it possible to use `require('internal/options')` in JS land to query the CLI options instead of using `process._breakFirstLine` and other underscored properties of `process` since we are now certain that these values should not be altered once the parsing is done and can be passed around in copies without locks. PR-URL: #24772 Reviewed-By: Anna Henningsen <[email protected]>

Instead of using a shared pointer of the entire debug option set, pass the parsed debug option to inspector classes by value because they are set once the CLI argument parsing is done. Add another shared pointer to HostPort being used by the inspector server, which is copied from the one in the debug options initially. The port of the shared HostPort is 9229 by default and can be specified as 0 initially but will be set to the actual port of the server once it starts listening. This makes the shared state clearer and makes it possible to use `require('internal/options')` in JS land to query the CLI options instead of using `process._breakFirstLine` and other underscored properties of `process` since we are now certain that these values should not be altered once the parsing is done and can be passed around in copies without locks. PR-URL: nodejs#24772 Reviewed-By: Anna Henningsen <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 1, 2018

joyeecheung added the inspector Issues and PRs related to the V8 inspector protocol label Dec 1, 2018

addaleax approved these changes Dec 1, 2018

View reviewed changes

src/node_options.h Outdated Show resolved Hide resolved

bnoordhuis reviewed Dec 2, 2018

View reviewed changes

joyeecheung commented Dec 2, 2018

View reviewed changes

src/node_process.cc Outdated Show resolved Hide resolved

joyeecheung force-pushed the inspector-options branch from d926a7c to 80abade Compare December 4, 2018 10:20

joyeecheung mentioned this pull request Dec 5, 2018

CI failures: 20181201-20181205 nodejs/reliability#19

Closed

joyeecheung force-pushed the inspector-options branch from 80abade to c631a1a Compare December 6, 2018 22:38

joyeecheung closed this Dec 8, 2018

BethGriggs mentioned this pull request Dec 18, 2018

V11.5.0 proposal #25102

Merged

beevelop mentioned this pull request Dec 18, 2018

2018-12-18 Version 11.5.0 (Current) @BethGriggs beevelop/docker-android-nodejs#272

Closed

BethGriggs added the backport-requested-v10.x label Jul 2, 2019

pluris mentioned this pull request Sep 25, 2023

src: modify int to uint16_t for HostPort #49845

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inspector: split the HostPort being used and the one parsed from CLI #24772

inspector: split the HostPort being used and the one parsed from CLI #24772

joyeecheung commented Dec 1, 2018

nodejs-github-bot commented Dec 1, 2018

joyeecheung commented Dec 1, 2018

bnoordhuis Dec 2, 2018

joyeecheung Dec 2, 2018 •

edited

Loading

bnoordhuis Dec 2, 2018

joyeecheung Dec 2, 2018

bnoordhuis Dec 2, 2018

joyeecheung Dec 2, 2018 •

edited

Loading

joyeecheung commented Dec 3, 2018

joyeecheung commented Dec 3, 2018 •

edited

Loading

addaleax commented Dec 4, 2018

joyeecheung commented Dec 4, 2018

Trott commented Dec 4, 2018 •

edited

Loading

joyeecheung commented Dec 6, 2018

joyeecheung commented Dec 8, 2018

joyeecheung commented Dec 8, 2018

joyeecheung commented Dec 8, 2018

inspector: split the HostPort being used and the one parsed from CLI #24772

inspector: split the HostPort being used and the one parsed from CLI #24772

Conversation

joyeecheung commented Dec 1, 2018

Checklist

nodejs-github-bot commented Dec 1, 2018

joyeecheung commented Dec 1, 2018

bnoordhuis Dec 2, 2018

Choose a reason for hiding this comment

joyeecheung Dec 2, 2018 • edited Loading

Choose a reason for hiding this comment

bnoordhuis Dec 2, 2018

Choose a reason for hiding this comment

joyeecheung Dec 2, 2018

Choose a reason for hiding this comment

bnoordhuis Dec 2, 2018

Choose a reason for hiding this comment

joyeecheung Dec 2, 2018 • edited Loading

Choose a reason for hiding this comment

joyeecheung commented Dec 3, 2018

joyeecheung commented Dec 3, 2018 • edited Loading

addaleax commented Dec 4, 2018

joyeecheung commented Dec 4, 2018

Trott commented Dec 4, 2018 • edited Loading

joyeecheung commented Dec 6, 2018

joyeecheung commented Dec 8, 2018

joyeecheung commented Dec 8, 2018

joyeecheung commented Dec 8, 2018

joyeecheung Dec 2, 2018 •

edited

Loading

joyeecheung Dec 2, 2018 •

edited

Loading

joyeecheung commented Dec 3, 2018 •

edited

Loading

Trott commented Dec 4, 2018 •

edited

Loading