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

src: use option parser for expose_internals #12245

Merged
merged 2 commits into from
Apr 13, 2017
Merged

Conversation

sam-github
Copy link
Contributor

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

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

@mscdex mscdex added the process Issues and PRs related to the process subsystem. label Apr 5, 2017
src/node.cc Outdated
@@ -3279,6 +3280,7 @@ void SetupProcessObject(Environment* env,
READONLY_PROPERTY(process, "_forceRepl", True(env->isolate()));
}

// -r,--require
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

src/node.cc Outdated
@@ -3334,6 +3338,11 @@ void SetupProcessObject(Environment* env,
READONLY_PROPERTY(process, "_debugWaitConnect", True(env->isolate()));
}

// --expose_internals,--expose-internals
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

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

src/node.cc Outdated
// --expose_internals,--expose-internals
if (expose_internals) {
READONLY_PROPERTY(process, "_exposeInternals", True(env->isolate()));
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

src/node.cc Outdated
@@ -3334,6 +3338,13 @@ void SetupProcessObject(Environment* env,
READONLY_PROPERTY(process, "_debugWaitConnect", True(env->isolate()));
}

// --expose_internals, --expose-internals
// Note that this is not exposed as a process property, it is deleted when
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell PTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -5,3 +5,4 @@ require('../common');
const assert = require('assert');

assert.strictEqual(typeof require('internal/freelist').FreeList, 'function');
assert(!('_exposeInternals' in process), 'no process property is leaked');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@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.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

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

@sam-github
Copy link
Contributor Author

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

@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
Copy link
Member

jasnell commented Apr 11, 2017

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

OK, I'll go look

@sam-github
Copy link
Contributor Author

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 <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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 <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@sam-github sam-github merged commit 8086cb6 into master Apr 13, 2017
@sam-github sam-github deleted the use-expose-internals branch April 13, 2017 18:51
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

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

@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

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.

sam-github added a commit to sam-github/node that referenced this pull request Jul 25, 2017
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 <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
sam-github added a commit to sam-github/node that referenced this pull request Jul 25, 2017
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 <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@sam-github
Copy link
Contributor Author

backported: #14483

MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
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 <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
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 <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
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 <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 14, 2017
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 <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
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 <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
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 <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
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++. lib / src Issues and PRs related to general changes in the lib or src directory. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants