child_process: do not ignore proto values of env #18210

apapirovski · Jan 17, 2018

This reverts part of the behaviour introduced in a recent PR, and updates the test. Without this change, CitGM and other packages are broken. I would like to have this fast-tracked (like, land within an hour or less) because as things are, CitGM is completely broken.

Also, I would like to propose, that going forward all changes that are labeled as semver-minor/major/patch MUST have a CitGM run to land. If any issues are found, collaborators & TSC should decide whether those are blocking or not. In either case, an effort should be made to fix the impacted modules before the PR lands — or at the very least in cases where those modules are integral to the Node.js core project.

Refs: 85739b6
Fixes: nodejs/citgm#536

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

child_process, test

evanlucas · Jan 17, 2018

Should we not revert this entirely?

apapirovski · Jan 17, 2018

Should we not revert this entirely?

We don't need to. Just the prototype portion is excluded. To be honest, IMO that portion of the change kind of snuck through because the documented part is only re: undefined values.

apapirovski · Jan 17, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/12586/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1206/

(Does CitGM use the newly built node version to run itself? If not, that might not work so well...)

joyeecheung · Jan 17, 2018

@apapirovski I think it does because the previous citgm errors seemed to be caused by citgm itself, not the modules that it was testing.

richardlau · Jan 17, 2018

Note that some of the modules tested by CitGM will fail on master: nodejs/citgm#517

evanlucas · Jan 17, 2018

evanlucas approved these changes Jan 17, 2018

View changes

jasnell · Jan 17, 2018

jasnell approved these changes Jan 17, 2018

View changes

addaleax · Jan 17, 2018

addaleax approved these changes Jan 17, 2018

View changes

addaleax · Jan 17, 2018

To be clear, my approval is not just +1 to landing this but also +1 to considering this the right behaviour.

ljharb · Jan 17, 2018

Why is this conceptually the right behavior? By far, the language conventions when reflecting on objects are for only including own enumerable properties - eg Object.keys/values/entries/assign and object spread.

mcollina · Jan 18, 2018

mcollina approved these changes Jan 18, 2018

View changes

LGTM with a nit.

mcollina · Jan 18, 2018

lib/child_process.js

@@ -504,7 +504,7 @@ function normalizeSpawnArguments(file, args, options) {
  var env = options.env || process.env;
  var envPairs = [];

-  for (const key of Object.keys(env)) {
+  for (var key in env) {


I would add a comment that flag that states that copying the prototype values is intentional.

I added a comment to the commit before landing.

mcollina · Jan 18, 2018

I think it would be breakage without any reason, and it would not have landed if we were aware that it would break CITGM. Moreover, the change was introduced to fix #15087, which is definitely not about the prototype chain.

@ljharb if you want to pursue this specific change, can you send a different PR so it can be discussed separately?

targos · Jan 18, 2018

targos approved these changes Jan 18, 2018

View changes

apapirovski · Jan 18, 2018

Landed in 38ee25e

ljharb · Jan 18, 2018

@mcollina totally understand that the change was unintentional and breaking, so of course it should be reverted ASAP.

Philosophically tho, im not sure why it’s desired. Before i invest time in pursuing a change, I’d love to understand why anyone thinks iterating the prototype is a good idea. @addaleax, can you (or anyone else if they share the opinion) elaborate on why?

addaleax · Jan 18, 2018

@ljharb I’m not sure, but it does feel more natural to me – in my head, the child’s process.env should match what the parent passed in for env as closely as possible. (But don’t worry, I wouldn’t fight over this if somebody opened a deliberate semver-major PR to change it again.)

joyeecheung · Jan 18, 2018

We should look into why CITGM (or the modules it uses) relies on the prototype properties passed as env though.

mcollina · Jan 18, 2018

It’d be trivial to change: https://github.com/nodejs/citgm/blob/master/lib/create-options.js.

jasnell · Jan 18, 2018

In general, I'd be fine with limiting it to own properties only in a semver-major.

apapirovski · Jan 18, 2018

What's not trivial is that there's an absolute ton of user-land code that relies on this behaviour:

https://github.com/search?q=object.create%28process.env%29&type=Code&utf8=%E2%9C%93
https://github.com/search?utf8=%E2%9C%93&q=%22object.create%28process.env%29%22&type=Code (this is a bit more precise — less unrelated code — but misses some of the instances that the above search finds)

It appears to be a VERY common pattern.

ljharb · Jan 18, 2018

That’s indeed a strong argument that it’s not worth the change, even if i think that “own keys only” is the correct/better conceptual model.

apapirovski added the fast-track label Jan 17, 2018

apapirovski requested review from jasnell, addaleax, benjamingr and targos Jan 17, 2018

nodejs-github-bot added the child_process label Jan 17, 2018

apapirovski referenced this pull request Jan 17, 2018
Closed
CITGM seems to be broken on master #536

apapirovski added the semver-major label Jan 17, 2018

joyeecheung referenced this pull request Jan 17, 2018
Open
Check that a semver-major PR has at least a CITGM CI run #141

apapirovski referenced this pull request Jan 17, 2018
Closed
child_process: ignore undef/proto values of env #15089
4 of 4 tasks complete

joyeecheung added the author ready label Jan 18, 2018

mcollina referenced this pull request Jan 18, 2018
Closed
stream: check type and range of highWaterMark #18098
3 of 4 tasks complete

apapirovski closed this Jan 18, 2018

apapirovski deleted the apapirovski:fix-env-prototype-key-values branch Jan 18, 2018

addaleax removed the author ready label Jan 18, 2018

nodejs/node

Join GitHub today

child_process: do not ignore proto values of env #18210

Conversation

apapirovski commented Jan 17, 2018 • edited

Checklist

Affected core subsystem(s)

apapirovski added the fast-track label Jan 17, 2018

apapirovski requested review from jasnell, addaleax, benjamingr and targos Jan 17, 2018

nodejs-github-bot added the child_process label Jan 17, 2018

This comment has been minimized.

evanlucas commented Jan 17, 2018

apapirovski referenced this pull request Jan 17, 2018

CITGM seems to be broken on master #536

This comment has been minimized.

apapirovski commented Jan 17, 2018

apapirovski added the semver-major label Jan 17, 2018

joyeecheung referenced this pull request Jan 17, 2018

Check that a semver-major PR has at least a CITGM CI run #141

This comment has been minimized.

apapirovski commented Jan 17, 2018 • edited

This comment has been minimized.

joyeecheung commented Jan 17, 2018

This comment has been minimized.

richardlau commented Jan 17, 2018

evanlucas approved these changes Jan 17, 2018 View changes

apapirovski referenced this pull request Jan 17, 2018

child_process: ignore undef/proto values of env #15089

jasnell approved these changes Jan 17, 2018 View changes

addaleax approved these changes Jan 17, 2018 View changes

This comment has been minimized.

addaleax commented Jan 17, 2018

This comment has been minimized.

ljharb commented Jan 17, 2018

joyeecheung added the author ready label Jan 18, 2018

mcollina approved these changes Jan 18, 2018 View changes

mcollina left a comment

This comment has been minimized.

mcollina Jan 18, 2018

This comment has been minimized.

apapirovski Jan 18, 2018

This comment has been minimized.

mcollina commented Jan 18, 2018

targos approved these changes Jan 18, 2018 View changes

mcollina referenced this pull request Jan 18, 2018

stream: check type and range of highWaterMark #18098

This comment has been minimized.

apapirovski commented Jan 18, 2018

apapirovski closed this Jan 18, 2018

apapirovski deleted the apapirovski:fix-env-prototype-key-values branch Jan 18, 2018

apapirovski added a commit that referenced this pull request Jan 18, 2018

addaleax removed the author ready label Jan 18, 2018

This comment has been minimized.

ljharb commented Jan 18, 2018

This comment has been minimized.

addaleax commented Jan 18, 2018

This comment has been minimized.

joyeecheung commented Jan 18, 2018

This comment has been minimized.

mcollina commented Jan 18, 2018

This comment has been minimized.

jasnell commented Jan 18, 2018

This comment has been minimized.

apapirovski commented Jan 18, 2018 • edited

This comment has been minimized.

ljharb commented Jan 18, 2018

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

apapirovski commented Jan 17, 2018 •

edited

apapirovski commented Jan 17, 2018 •

edited

evanlucas approved these changes Jan 17, 2018

View changes

jasnell approved these changes Jan 17, 2018

View changes

addaleax approved these changes Jan 17, 2018

View changes

mcollina approved these changes Jan 18, 2018

View changes

targos approved these changes Jan 18, 2018

View changes

apapirovski commented Jan 18, 2018 •

edited