src: use option parser for expose_internals#12245

sam-github · 2017-04-05T21:14:01Z

bootstrap_node.js was directly parsing process.execArgv to see if
internals should be exposed, even though the argv was already parsed by
node. This is unusual and unnecessary, change it to set the option value
from the parser onto the process object, as is done for the other CLI
options.

Checklist

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

Affected core subsystem(s)

src

sam-github · 2017-04-05T21:15:31Z

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

richardlau · 2017-04-05T22:41:19Z

src/node.cc

 READONLY_PROPERTY(process, "_forceRepl", True(env->isolate()));
 }

+// -r,--require


Actually can you add a space here after the comma to be consistent with the other existing comments?

done, thanks (also, fixed up the existing comment which did not use a space)

richardlau · 2017-04-05T22:41:41Z

src/node.cc

 READONLY_PROPERTY(process, "_debugWaitConnect", True(env->isolate()));
 }

+// --expose_internals,--expose-internals


jasnell

In addition to the other issue mentioned, this should have a minimal test to verify that the property is set.

jasnell · 2017-04-06T02:10:09Z

src/node.cc

+// --expose_internals,--expose-internals
+if (expose_internals){
+READONLY_PROPERTY(process, "_exposeInternals", True(env->isolate()));
+ }


Would prefer to use process.binding('config') for this rather than adding a new _-prefixed property off process

PTAL, there is no new property created off of process (its deleted later). I'll add an assertion of that fact to the existing test.

sam-github · 2017-04-06T19:08:34Z

src/node.cc

 }

+// --expose_internals, --expose-internals
+// Note that this is not exposed as a process property, it is deleted when


@jasnell PTAL

not sure the note is necessary, many setup properties do this

I didn't think it was necessary either, but it confused @jasnell - James, what do you think?

I want confused. I'm not fond of this pattern. Exposing via process.binding('config') is preferable to me.

sam-github · 2017-04-06T19:09:56Z

test/parallel/test-internal-modules-expose.js

 constassert=require('assert');

 assert.strictEqual(typeofrequire('internal/freelist').FreeList,'function');
+assert(!('_exposeInternals'inprocess),'no process property is leaked');


@jasnell PTAL, I deliberately am making no change to the process properties (for cleanliness, and because it may potentically be backwards incompat, and also because its unnecessary, we can find if internal modules were exposed pretty easily by just trying to require one and seeing what happens, which is unusual to want to know, but possible).

see
node/src/node.cc
Lines 1219 to 1221 in 8460284
env->process_object()->Delete(
env->context(),
FIXED_ONE_BYTE_STRING(args.GetIsolate(), "_setupPromises")).FromJust();
, this is already common practice, just usually from C++.

addaleax · 2017-04-10T14:24:40Z

@sam-github This might also need a rebase after landing #12241.
@jasnell Care to take another look? I think everything here has been addressed/answered.

jasnell

@addaleax I'd still prefer the process.binding('config') change to be honest.

sam-github · 2017-04-10T18:16:16Z

@jasnell what change? You saw the property is never visible, and unit test to that effect? You want the property put somewhere else for its brief lifetime?

sam-github · 2017-04-10T18:28:48Z

@jasnell I somehow missed your earlier comment, sorry.

You actually want this exposed as a new config property? I don't think I'm on-board for that. I'm trying to factor out an oddity where process.argv is re-parsed after it was already parsed, which strikes me as undesireable, not add new APIs that haven't been requested.

process.config in particular contains variables set by ./configure at this moment, it doesn't seem like the place to start adding command line arg flags. Maybe a process.options that contains the values of all the CLI options and flags would be an interesting feature, though I haven't seen that requested as a feature.

jasnell · 2017-04-11T10:12:45Z

No, not process.config, process.binding('config')... look at node_config.cc. It is an internal only mechanism for exposing configuration flags so that we can avoid hanging things off process and deleting them later. It was introduced specifically because process.config is unreliable and adding things to process is messy.

sam-github · 2017-04-11T14:18:58Z

OK, I'll go look

sam-github · 2017-04-12T21:05:25Z

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

A few of the CLI option values exposed as properties on the process object were missing a comment, fix this. PR-URL: #12245 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

bootstrap_node.js was directly parsing process.execArgv to see if internals should be exposed, even though the argv was already parsed by node. This is unusual and unnecessary, change it to set the option value from the parser onto the config binding. PR-URL: #12245 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

gibfahn · 2017-06-18T12:42:20Z

@sam-github when you merge multiple commits, could you still comment with Landed in HASH1 HASH2 as per the Collaborator Guide?

gibfahn · 2017-06-18T12:43:36Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

A few of the CLI option values exposed as properties on the process object were missing a comment, fix this. PR-URL: nodejs#12245 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

bootstrap_node.js was directly parsing process.execArgv to see if internals should be exposed, even though the argv was already parsed by node. This is unusual and unnecessary, change it to set the option value from the parser onto the config binding. PR-URL: nodejs#12245 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

sam-github · 2017-07-25T17:51:09Z

backported: #14483

A few of the CLI option values exposed as properties on the process object were missing a comment, fix this. Backport-PR-URL: #14483 PR-URL: #12245 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

bootstrap_node.js was directly parsing process.execArgv to see if internals should be exposed, even though the argv was already parsed by node. This is unusual and unnecessary, change it to set the option value from the parser onto the config binding. Backport-PR-URL: #14483 PR-URL: #12245 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

A few of the CLI option values exposed as properties on the process object were missing a comment, fix this. Backport-PR-URL: #14483 PR-URL: #12245 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

bootstrap_node.js was directly parsing process.execArgv to see if internals should be exposed, even though the argv was already parsed by node. This is unusual and unnecessary, change it to set the option value from the parser onto the config binding. Backport-PR-URL: #14483 PR-URL: #12245 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

A few of the CLI option values exposed as properties on the process object were missing a comment, fix this. Backport-PR-URL: #14483 PR-URL: #12245 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

bootstrap_node.js was directly parsing process.execArgv to see if internals should be exposed, even though the argv was already parsed by node. This is unusual and unnecessary, change it to set the option value from the parser onto the config binding. Backport-PR-URL: #14483 PR-URL: #12245 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[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 Apr 5, 2017

sam-github mentioned this pull request Apr 5, 2017
src: allow CLI args in env with NODE_OPTIONS #12028
Merged
4 tasks

mscdex added the process Issues and PRs related to the process subsystem. label Apr 5, 2017

richardlau approved these changes Apr 5, 2017
View reviewed changes

richardlau requested changes Apr 5, 2017
View reviewed changes

jasnell requested changes Apr 6, 2017
View reviewed changes

sam-github force-pushed the use-expose-internals branch from ed9c2e6 to 8675a23Compare April 6, 2017 19:02

richardlau approved these changes Apr 6, 2017
View reviewed changes

sam-github commented Apr 6, 2017
View reviewed changes

addaleax approved these changes Apr 9, 2017
View reviewed changes

cjihrig approved these changes Apr 9, 2017
View reviewed changes

jasnell reviewed Apr 10, 2017
View reviewed changes

sam-github force-pushed the use-expose-internals branch from 4e22b98 to 913a194Compare April 10, 2017 18:20

sam-github force-pushed the use-expose-internals branch from 913a194 to 8b07404Compare April 10, 2017 18:32

sam-github force-pushed the use-expose-internals branch from 88ca10f to d746bcaCompare April 11, 2017 21:23

jasnell approved these changes Apr 12, 2017
View reviewed changes

addaleax approved these changes Apr 12, 2017
View reviewed changes

sam-github force-pushed the use-expose-internals branch from d746bca to 1b24364Compare April 12, 2017 21:04

sam-github added 2 commits April 13, 2017 07:09

sam-github force-pushed the use-expose-internals branch from 1b24364 to 8086cb6Compare April 13, 2017 14:10

sam-github merged commit 8086cb6 into masterApr 13, 2017

sam-github deleted the use-expose-internals branch April 13, 2017 18:51

jasnell mentioned this pull request May 11, 2017
8.0.0 Release Proposal #12220
Closed

gibfahn mentioned this pull request Jun 15, 2017
Auditing for 6.11.1 nodejs/Release#230
Closed
3 tasks

gibfahn added the backport-requested-v6.x label Jun 18, 2017

sam-github mentioned this pull request Jul 25, 2017
(v6.x backport) src: use option parser for expose_internals #14483
Closed
4 tasks

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Aug 14, 2017

MylesBorins mentioned this pull request Aug 16, 2017
v6.11.3-proposal #14852
Merged

	env->process_object()->Delete(
	env->context(),
	FIXED_ONE_BYTE_STRING(args.GetIsolate(), "_setupPromises")).FromJust();

Uh oh!

src: use option parser for expose_internals#12245

src: use option parser for expose_internals #12245

Uh oh!

Conversation

sam-github commented Apr 5, 2017

Checklist

Affected core subsystem(s)

Uh oh!

sam-github commented Apr 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Apr 10, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

sam-github commented Apr 10, 2017

Uh oh!

sam-github commented Apr 10, 2017

Uh oh!

jasnell commented Apr 11, 2017

Uh oh!

sam-github commented Apr 11, 2017

Uh oh!

sam-github commented Apr 12, 2017

Uh oh!

gibfahn commented Jun 18, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibfahn commented Jun 18, 2017

Uh oh!

sam-github commented Jul 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

gibfahn commented Jun 18, 2017•
edited
Loading