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: add process.binding('config') #6266

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
7 participants
@jasnell
Copy link
Member

commented Apr 19, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

process

Description of change

It turns out that userland likes to override process.config with their own stuff. If we want to be able to depend on it in any way, we need our own internal reference. It's still not read-only but that should be ok.

  • Documents that process.config can and likely will be overwritten
  • Adds process.binding('config')
  • Adds the icu/intl configs as flags

/cc @Fishrock123 @srl295

@jasnell jasnell added the process label Apr 19, 2016

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

It's still not read-only but that should be ok.

Uh, that is not ok. We aren't just going to expose an alias on process, that's only more surface bloat. Just make it an internal/.

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2016

Doesn't really seem like an issue to me but very well. Made it internal. PTAL

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

@jasnell Should this also contain a change changing our internals to use this? It's not very useful to land something that no-one uses. :)

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2016

Currently in master we only use process.config directly in tests, which would not be affected. In v4 and v5 we have code that uses process.config in lib but that was refactored out in master. There is one open PR from @srl295 that would add code back into lib that would depend on this that would need to be updated before it lands.

If/when this lands, I intend to backport it to resolve the existing regressions that have occurred in v5 and v4.

@@ -60,7 +60,7 @@ function setupConfig(_source) {
.replace(/"/g, '\\"')
.replace(/'/g, '"');

process.config = JSON.parse(config, function(key, value) {
process.config = exports.config = JSON.parse(config, (key, value) => {

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Apr 19, 2016

Member

This line doesn't make a copy, process.config and exports.config point to the same object.

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 19, 2016

Author Member

Yep. Modification is still possible. The problem we're having in userland is with code that does stuff like process.config = {}, essentially overwriting everything. The userland code that extends the object hasn't been a problem that I can determine. This is an attempt at doing the simplest thing first. If we need to lock it down more, then let's do so.

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Apr 19, 2016

Member

If we want to be robust against users clobbering the runtime environment, then we shouldn't take half measures; either we guard against it or we don't.

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2016

The other obvious option here is to set a rule that we simply should not be depending on this in core code at all. If some bit of code needs to know if a config flag was set on compile time, then some other mechanism should be used to expose (perhaps an internal/flags module) only the flags we specifically need to check. We can add a lint rule that complains if process.config is used in lib.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

That's sounds fine. As long as the net effect is the same.

@trevnorris

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2016

Would it be considered a breaking change to make or read only? Native modules use default_configuration to determine what binary to run. Basically, I can think of reasons to make it read only, but not any to leave it mutable.

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2016

There are a number of userland modules that either replace process.config outright with alternative objects or that mutate it's properties. Making the entire process.config read only breaks a bunch of stuff.. so yeah, it would definitely be a breaking semver-major.

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2016

Ok, so looking it over I can see a couple of possible routes. Here's one that I just stepped through the implementation on:

  • Add src/node_config.h and src/node_config.cc. These create a new process.binding('_config') that returns an object with selected constant boolean flags on it that are explicitly set only when we know we are actually going to need them. For instance, #4253 needs to know if small-icu is used and if the icu-data-dir flag has been set... so we'd have two boolean exposed on process.binding('_config') like hasSmallICU and usingICUDataDir. The point is, we're not setting flag constants for every config flag, just the ones we know we're going to need.
  • We treat process.binding('_config') as an internal only thing. There's no guarantee that flags will stay there.
  • The upshot is, up to this point, we've been dropping these kinds of flags onto process which has the downside of exposing them to userland. Having the _config binding gives us a private place to expose those to internal code that doesn't pollute process any further.
  • The downside, of course, it's that it's a new process.binding(). I could, alternatively, add these flag constants to the existing constants binding if that would be more desirable.

Thoughts? Other solutions?

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

There's never been any stability guarantee for process.binding() so that sounds fine to me.

@jasnell jasnell force-pushed the jasnell:process-config branch from 26a503e Apr 19, 2016

@jasnell jasnell changed the title process: set an internal copy of process._config src: add process.binding('_config') Apr 19, 2016

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2016

@bnoordhuis, @srl295, @trevnorris, @Fishrock123 ... PTAL. Reworked this completely to use the proposed process.binding('_config') instead of touching process.config at all

@vkurchatkin

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

@jasnell why underscore? Binding is already private

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2016

To ahem underscore the point? ;) Honestly it's just what I chose in the moment. You're right that it's redundant.

@jasnell jasnell referenced this pull request Apr 20, 2016

Closed

module: expose `Module._runInThisContext` #6288

3 of 3 tasks complete

@jasnell jasnell force-pushed the jasnell:process-config branch 2 times, most recently Apr 20, 2016

@jasnell jasnell changed the title src: add process.binding('_config') src: add process.binding('config') Apr 20, 2016

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2016

@vkurchatkin ... updated to remove the underscore!

@targos

View changes

node.gyp Outdated
@@ -132,6 +132,7 @@
'src/node_buffer.cc',
'src/node_constants.cc',
'src/node_contextify.cc',
'src/node_config.cc',

This comment has been minimized.

Copy link
@targos

targos Apr 20, 2016

Member

nit: this should go between buffer and constants

@targos

View changes

src/node_config.cc Outdated
// The config binding is used to provide an internal view of compile or runtime
// config options that are required internally by lib/*.js code. This is an
// alternative to dropping additional properties onto the process object as
// has ben the practice previously in node.cc.

This comment has been minimized.

Copy link
@targos

targos Apr 20, 2016

Member

has been

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

Can we put the place we are actually going to use this into this PR? This is kinda pointless without being coupled to it. :)

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2016

@Fishrock123 ... that's a separate issue (#4253 (comment)) that makes much more sense keeping separated. Also, I'd prefer to keep this separate as I intend to backport it to v5 and v4 to address bugs there.

@bnoordhuis

View changes

lib/internal/process.js Outdated
@@ -60,7 +60,7 @@ function setupConfig(_source) {
.replace(/"/g, '\\"')
.replace(/'/g, '"');

process.config = JSON.parse(config, function(key, value) {
process.config = JSON.parse(config, (key, value) => {

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Apr 20, 2016

Member

Unrelated change now.

This comment has been minimized.

Copy link
@jasnell

jasnell Apr 20, 2016

Author Member

ah, right +1

@jasnell jasnell closed this Apr 21, 2016

joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

doc: note that process.config can and will be changed
PR-URL: nodejs#6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

src: add process.binding('config')
It turns out that userland likes to override process.config with
their own stuff. If we want to be able to depend on it in any way,
we need our own internal mechanism.

This adds a new private process.binding('config') that is
intended to serve as a container for internal flags and compile
time configs that need to be passed on to the JS layer.

PR-URL: nodejs#6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

src: add intl and icu configs to process.binding('config')
PR-URL: nodejs#6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell added a commit that referenced this pull request Apr 26, 2016

doc: note that process.config can and will be changed
PR-URL: #6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell added a commit that referenced this pull request Apr 26, 2016

src: add process.binding('config')
It turns out that userland likes to override process.config with
their own stuff. If we want to be able to depend on it in any way,
we need our own internal mechanism.

This adds a new private process.binding('config') that is
intended to serve as a container for internal flags and compile
time configs that need to be passed on to the JS layer.

PR-URL: #6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell added a commit that referenced this pull request Apr 26, 2016

src: add intl and icu configs to process.binding('config')
PR-URL: #6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

@jasnell do we still want to backport this to v4.x?

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Jun 30, 2016

Oh right! I'll do that.
On Jun 30, 2016 4:22 PM, "Myles Borins" notifications@github.com wrote:

@jasnell https://github.com/jasnell do we still want to backport this
to v4.x?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6266 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAa2eYF0iQmHJmDRKEThtxcXcxlQ5iHsks5qRE-wgaJpZM4IKVnI
.

jasnell added a commit to jasnell/node that referenced this pull request Jul 5, 2016

doc: note that process.config can and will be changed
PR-URL: nodejs#6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell added a commit to jasnell/node that referenced this pull request Jul 5, 2016

src: add process.binding('config')
It turns out that userland likes to override process.config with
their own stuff. If we want to be able to depend on it in any way,
we need our own internal mechanism.

This adds a new private process.binding('config') that is
intended to serve as a container for internal flags and compile
time configs that need to be passed on to the JS layer.

PR-URL: nodejs#6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Jul 11, 2016

doc: note that process.config can and will be changed
PR-URL: #6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Jul 11, 2016

src: add process.binding('config')
It turns out that userland likes to override process.config with
their own stuff. If we want to be able to depend on it in any way,
we need our own internal mechanism.

This adds a new private process.binding('config') that is
intended to serve as a container for internal flags and compile
time configs that need to be passed on to the JS layer.

PR-URL: #6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Jul 11, 2016

doc: note that process.config can and will be changed
PR-URL: #6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Jul 11, 2016

src: add process.binding('config')
It turns out that userland likes to override process.config with
their own stuff. If we want to be able to depend on it in any way,
we need our own internal mechanism.

This adds a new private process.binding('config') that is
intended to serve as a container for internal flags and compile
time configs that need to be passed on to the JS layer.

PR-URL: #6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Jul 12, 2016

doc: note that process.config can and will be changed
PR-URL: #6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Jul 12, 2016

src: add process.binding('config')
It turns out that userland likes to override process.config with
their own stuff. If we want to be able to depend on it in any way,
we need our own internal mechanism.

This adds a new private process.binding('config') that is
intended to serve as a container for internal flags and compile
time configs that need to be passed on to the JS layer.

PR-URL: #6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@MylesBorins MylesBorins referenced this pull request Jul 12, 2016

Merged

v4.5.0 proposal #7688

MylesBorins added a commit that referenced this pull request Jul 14, 2016

doc: note that process.config can and will be changed
PR-URL: #6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Jul 14, 2016

src: add process.binding('config')
It turns out that userland likes to override process.config with
their own stuff. If we want to be able to depend on it in any way,
we need our own internal mechanism.

This adds a new private process.binding('config') that is
intended to serve as a container for internal flags and compile
time configs that need to be passed on to the JS layer.

PR-URL: #6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Jul 14, 2016

doc: note that process.config can and will be changed
PR-URL: #6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Jul 14, 2016

src: add process.binding('config')
It turns out that userland likes to override process.config with
their own stuff. If we want to be able to depend on it in any way,
we need our own internal mechanism.

This adds a new private process.binding('config') that is
intended to serve as a container for internal flags and compile
time configs that need to be passed on to the JS layer.

PR-URL: #6266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.