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

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

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

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 <anna@addaleax.net>

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 <anna@addaleax.net>

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

Uh oh!

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

Uh oh!

Conversation

joyeecheung commented Dec 1, 2018

Checklist

Uh oh!

nodejs-github-bot commented Dec 1, 2018

Uh oh!

joyeecheung commented Dec 1, 2018

Uh oh!

Uh oh!

Uh oh!

bnoordhuis Dec 2, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung Dec 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Dec 2, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung Dec 2, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Dec 2, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung Dec 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joyeecheung commented Dec 3, 2018

Uh oh!

joyeecheung commented Dec 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Dec 4, 2018

Uh oh!

joyeecheung commented Dec 4, 2018

Uh oh!

Trott commented Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Dec 6, 2018

Uh oh!

joyeecheung commented Dec 8, 2018

Uh oh!

joyeecheung commented Dec 8, 2018

Uh oh!

joyeecheung commented Dec 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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