src: add process.binding('config') #6266

jasnell · 2016-04-19T04:04:21Z

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

Fishrock123 · 2016-04-19T04:13:26Z

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 · 2016-04-19T04:21:53Z

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

Fishrock123 · 2016-04-19T04:52:41Z

@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 · 2016-04-19T05:01:05Z

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.

bnoordhuis · 2016-04-19T08:31:07Z

lib/internal/process.js

@@ -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 line doesn't make a copy, process.config and exports.config point to the same object.

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.

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 · 2016-04-19T15:34:36Z

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 · 2016-04-19T15:50:51Z

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

trevnorris · 2016-04-19T16:12:09Z

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 · 2016-04-19T16:24:23Z

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 · 2016-04-19T18:29:00Z

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, nodejs/node#3111 - v8BreakIterator disablement #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 · 2016-04-19T20:06:11Z

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

jasnell · 2016-04-19T23:18:25Z

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

vkurchatkin · 2016-04-20T00:40:42Z

@jasnell why underscore? Binding is already private

jasnell · 2016-04-20T00:42:37Z

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

jasnell · 2016-04-20T04:17:01Z

@vkurchatkin ... updated to remove the underscore!

targos · 2016-04-20T04:39:15Z

node.gyp

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


nit: this should go between buffer and constants

Fishrock123 · 2016-04-20T04:56:02Z

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 · 2016-04-20T05:15:08Z

@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 · 2016-04-20T09:28:47Z

lib/internal/process.js

@@ -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) => {


Unrelated change now.

ah, right +1

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

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>

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

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

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>

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

MylesBorins · 2016-06-30T23:22:17Z

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

jasnell · 2016-06-30T23:32:53Z

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
.