child_process: do not ignore proto values of env by apapirovski · Pull Request #18210 · nodejs/node

apapirovski · 2018-01-17T18:34:03Z

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

This reverts this behaviour introduced in a recent PR, and updates the test. Without this change, CitGM and other packages are broken. Refs: nodejs@85739b6

evanlucas · 2018-01-17T18:36:00Z

Should we not revert this entirely?

apapirovski · 2018-01-17T18:37:27Z

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 · 2018-01-17T18:49:05Z

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 · 2018-01-17T18:58:22Z

@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 · 2018-01-17T19:25:36Z

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

addaleax · 2018-01-17T22:40:52Z

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

ljharb · 2018-01-17T22:43:51Z

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

LGTM with a nit.

mcollina · 2018-01-18T09:37:02Z

lib/child_process.js

  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 · 2018-01-18T09:45:34Z

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?

apapirovski · 2018-01-18T13:04:48Z

Landed in 38ee25e

This reverts this behaviour introduced in a recent PR, and updates the test. Without this change, CitGM and other packages are broken. PR-URL: #18210 Fixes: nodejs/citgm#536 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

ljharb · 2018-01-18T16:14:11Z

@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 · 2018-01-18T18:01:26Z

@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 · 2018-01-18T18:56:14Z

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

mcollina · 2018-01-18T19:32:52Z

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

jasnell · 2018-01-18T19:35:22Z

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

apapirovski · 2018-01-18T19:36:09Z

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 · 2018-01-18T19:36:57Z

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.

This reverts this behaviour introduced in a recent PR, and updates the test. Without this change, CitGM and other packages are broken. PR-URL: nodejs#18210 Fixes: nodejs/citgm#536 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

child_process: do not ignore proto values of env

b36864a

This reverts this behaviour introduced in a recent PR, and updates the test. Without this change, CitGM and other packages are broken. Refs: nodejs@85739b6

apapirovski added the fast-track PRs that do not need to wait for 72 hours to land. label Jan 17, 2018

apapirovski requested review from addaleax, benjamingr, jasnell and targos January 17, 2018 18:34

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Jan 17, 2018

apapirovski mentioned this pull request Jan 17, 2018

CITGM seems to be broken on master nodejs/citgm#536

Closed

apapirovski added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 17, 2018

joyeecheung mentioned this pull request Jan 17, 2018

Check that a semver-major PR has at least a CITGM CI run nodejs/node-core-utils#141

Closed

evanlucas approved these changes Jan 17, 2018

View reviewed changes

apapirovski mentioned this pull request Jan 17, 2018

child_process: ignore undef/proto values of env #15089

Closed

4 tasks

jasnell approved these changes Jan 17, 2018

View reviewed changes

addaleax approved these changes Jan 17, 2018

View reviewed changes

joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 18, 2018

mcollina approved these changes Jan 18, 2018

View reviewed changes

targos approved these changes Jan 18, 2018

View reviewed changes

mcollina mentioned this pull request Jan 18, 2018

stream: check type and range of highWaterMark #18098

Closed

4 tasks

apapirovski closed this Jan 18, 2018

apapirovski deleted the fix-env-prototype-key-values branch January 18, 2018 13:04

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 18, 2018

Uh oh!

Conversation

apapirovski commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

evanlucas commented Jan 17, 2018

Uh oh!

apapirovski commented Jan 17, 2018

Uh oh!

apapirovski commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Jan 17, 2018

Uh oh!

richardlau commented Jan 17, 2018

Uh oh!

addaleax commented Jan 17, 2018

Uh oh!

ljharb commented Jan 17, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

apapirovski Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jan 18, 2018

Uh oh!

apapirovski commented Jan 18, 2018

Uh oh!

ljharb commented Jan 18, 2018

Uh oh!

addaleax commented Jan 18, 2018

Uh oh!

joyeecheung commented Jan 18, 2018

Uh oh!

mcollina commented Jan 18, 2018

Uh oh!

jasnell commented Jan 18, 2018

Uh oh!

apapirovski commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb commented Jan 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

apapirovski commented Jan 17, 2018 •

edited

Loading

apapirovski commented Jan 17, 2018 •

edited

Loading

apapirovski commented Jan 18, 2018 •

edited

Loading