child_process: ignore undef/proto values of env #15089

Gerhut · Aug 30, 2017

At present, undefined values of env option will be transferred as an "undefined" string value and values in the prototype will also be included, which are not usual behaviors.

Since non-string env values & prototype values are undocumented, this change may be treated as a bugfix or a breaking change.

Tested on Mac, Windows not yet.

Fixes: #15087

Checklist

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

Affected core subsystem(s)

child_process

benjamingr · Aug 30, 2017

benjamingr reviewed Aug 30, 2017

View changes

benjamingr · Aug 30, 2017

lib/child_process.js

@@ -467,7 +467,10 @@ function normalizeSpawnArguments(file, args, options) {
  var envPairs = [];

  for (var key in env) {


I think this should optimally be:

const envPairs = Object.keys(env).filter(Boolean).map(key => `${key}=${env[key]}`)

or something similar (ignore empty keys in addition to undefined ones.

I do think it's underspecified though. So I'll wait for someone who understands it better to weigh in.

OMG .filter(Boolean) 💘
I used to do .filter((x) => x), but have alway looked for a built-in.

P.S. @Gerhut in TF&I (node 8.3.0 and newer) for ... of loop are almost 10 time faster than for ... in loops, and it's supports Object.entries()

@refack but the call to Object.keys is itself about as slow as doing for... in - in this particular case I don't think performance is important given the time it takes to create a process and all 😅

@benjamingr agreed.
I'm just promoting TF&I good behaviour (no more CranksharfScript)

Since the code around still in legacy Syntax, should I be allowed to use modern syntax? If so I will modify them in modern syntax.

@Gerhut yes, in general new code should be written in new syntax.

However, for..in includes keys in prototype, and keys-in-prototype of env test exists at https://github.com/nodejs/node/blob/master/test/parallel/test-child-process-env.js#L31-L33 . Changing to Object.keys/entries will fail them

I think it would be good to remove that test case and only use Object.keys. There is not much reasoning in the original commit about why it was added but I think this is actually a faulty behavior.

Using filter(Boolean) would be a bad decision to use though as it would remove a empty string and any other falsy value as well.

benjamingr · Aug 30, 2017

Tagging as semver-major preemptively since this is a user facing behavioral change in an API marked 2 - Stable.

I don't feel strongly about it - I just don't want it to be missed.

benjamingr · Aug 30, 2017

@Gerhut thank you for the contribution. Just to explain the process at this point:

Collaborators are going to look at this pull request for around 48 hours to discuss the change.
People will comment on the actual code and might suggest changes.
People will comment on the merit of the actual changes and evaluate how they impact the ecosystem.

If at any point you would like assistance with the pull request or have any question about the process feel free to ask here. Welcome.

cjihrig · Aug 30, 2017

cjihrig reviewed Aug 30, 2017

View changes

Can you add documentation notes and function level changelog entries for this.

addaleax · Aug 31, 2017

LGTM as semver-major

BridgeAR · Aug 31, 2017

I personally am not convinced about this. Why should undefined and null be treated otherwise than any other primitive besides strings? They will all be coerced to a string and I can actually imagine that some people might explicitly pass undefined or null as env value. Having a implicit removal is not what I would call intuitive.
I would rather have a error thrown as a user to know what is happening and that it is better to always pass in values as string.

jasnell · Sep 1, 2017

I'm kind of half-way where @BridgeAR is. I would say null should be ToString'd and undefined should be ignored/omitted.

targos · Sep 1, 2017

I think it makes sense to ignore undefined too. No opinion on null.

addaleax · Sep 2, 2017

100 % totally official twitter poll: https://twitter.com/addaleax/status/903796395865452544

BridgeAR · Sep 3, 2017

Looks like the poll says null should be stringified and undefined should be removed. @Gerhut would you be so kind and update the PR accordingly?

I personally still think we should not remove it but throw an error instead and the poll did not cover that part but it seems like most people agree on removing it, so I will not hold this off.

refack · Sep 3, 2017

BTW: need to think about how to make this consistent with assignment (maybe in a future PR)

node/doc/api/process.md

Lines 827 to 842 in 98d8db3

    
           Assigning a property on `process.env` will implicitly convert the value 
        
           to a string. 
        
           Example: 
        
           ```js 
        
           process.env.test = null; 
        
           console.log(process.env.test); 
        
           // => 'null' 
        
           process.env.test = undefined; 
        
           console.log(process.env.test); 
        
           // => 'undefined' 
        
           ``` 
        
           Use `delete` to delete a property from `process.env`.

Gerhut · Sep 4, 2017

PR Updated.

Moreover, Would "ignore keys in prototype of env option" be another breaking change of bugfix? (See outdated review comment)

I just feel this change should be in another PR to discuss separately. There could be an use case that using process.env as a base environment variables and extend it in env option. It might using prototyping (env = Object.create(process.env); env.FOO='BAR'; spawn(exe, {env};).

BridgeAR · Sep 11, 2017

BridgeAR reviewed Sep 11, 2017

View changes

In general LGTM but I would like my comment to be addressed.

BridgeAR · Sep 11, 2017

lib/child_process.js

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

-  for (var key in env) {
-    envPairs.push(key + '=' + env[key]);
+  for (const [key, value] of Object.entries(env)) {


I am not sure how fast Object.entries is and I somewhat expect it to be slower than e.g. using Object.keys or using for ... in with a Object.hasOwnProperty.call(env, key) check. The latter will definitely be faster in upcoming v8 versions but probably not right now.

BridgeAR · Sep 11, 2017

@Gerhut I think it is fine to keep it in this PR but it would be just as fine to move it to another one!

If this is kept in the PR the commit message has to be updated though. This has to be done one way or the other anyway though since it currently has information that is not necessary for landing. So I think it is your call. If you want to land another commit - feel free to open a second PR for that.

bnoordhuis · Sep 11, 2017

I'm -0. process.env and the .env option currently behave the same: values are tostring-ed. This PR introduces a discrepancy for no good reason I can see.

jasnell · Sep 15, 2017

jasnell approved these changes Sep 15, 2017

View changes

BridgeAR · Sep 19, 2017

BridgeAR dismissed their stale review Sep 19, 2017

Dimissed my ok as I am also -0 on the change but I do not want to block it either.

BridgeAR · Sep 22, 2017

This is semver major and needs some LGs @nodejs/tsc

It is still a tiny bit controversial in general though, so please read all the comments as well.

evanlucas · Sep 25, 2017

I'm on the fence on this one. Deviating from how null and undefined are handled in process.env makes this more confusing to me...

targos · Sep 25, 2017

I would be in favour of the same change for process.env

refack · Sep 25, 2017

refack reviewed Sep 25, 2017

View changes

refack · Sep 25, 2017

test/parallel/test-child-process-env.js

-  'HELLO': 'WORLD'
+  'HELLO': 'WORLD',
+  'UNDEFINED': undefined,
+  'NULL': null


Could you add 'EMPTY': ''

refack · Sep 25, 2017

refack reviewed Sep 25, 2017

View changes

refack · Sep 25, 2017

doc/api/child_process.md

@@ -412,7 +412,8 @@ Use `cwd` to specify the working directory from which the process is spawned.
 If not given, the default is to inherit the current working directory.

 Use `env` to specify environment variables that will be visible to the new
-process, the default is [`process.env`][].
+process, the default is [`process.env`][], `undefined` values in `env` will be


Suggestion keep the old line, and add a new sentence:

process, the default is [`process.env`][]. `undefined` values in `env` will be ignored.

addaleax · Dec 6, 2017

addaleax approved these changes Dec 6, 2017

View changes

addaleax · Dec 6, 2017

lib/child_process.js

-  for (var key in env) {
-    envPairs.push(`${key}=${env[key]}`);
+  for (const key of Object.keys(env)) {
+    const value = env[key]


Can you make sure running make lint passes?

Gerhut · Dec 6, 2017

Sorry for only ran make test-parallel these times.

targos · Jan 15, 2018

targos approved these changes Jan 15, 2018

View changes

targos · Jan 15, 2018

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/12531/

targos · Jan 15, 2018

Pushed a fix for Windows.
CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/14890/

Gerhut · Jan 15, 2018

Thanks

targos · Jan 15, 2018

Landed in 85739b6.
Thank you @Gerhut for your patience and first contribution to Node!

MylesBorins · Jan 17, 2018

In future please run CITGM before landing semver major commits

edit: for context this broke CITGM

targos · Jan 17, 2018

Sorry about that

apapirovski · Jan 17, 2018

I'll just leave a note here to anyone landing this in the future, it can't land without #18210.

nodejs-github-bot added the child_process label Aug 30, 2017

benjamingr added the semver-major label Aug 30, 2017

Gerhut force-pushed the Gerhut:master branch from fa5df44 to ea03fc8 Aug 31, 2017

Gerhut force-pushed the Gerhut:master branch from ea03fc8 to 8c10ccd Sep 4, 2017

Gerhut force-pushed the Gerhut:master branch from 8c10ccd to d3e7468 Sep 12, 2017

Gerhut changed the title ~~child_process: ignore null/undefined values of env~~ child_process: ignore undef/proto values of env Sep 12, 2017

Gerhut force-pushed the Gerhut:master branch from d3e7468 to c3be4f7 Sep 18, 2017

BridgeAR dismissed their stale review Sep 19, 2017

Gerhut force-pushed the Gerhut:master branch from c3be4f7 to cccd9d8 Sep 27, 2017

BridgeAR requested a review from nodejs/tsc Dec 6, 2017

Gerhut force-pushed the Gerhut:master branch from e566e1b to 5e37647 Dec 6, 2017

MylesBorins force-pushed the nodejs:master branch from b7405ab to 7f086dd Dec 8, 2017

rvagg referenced this pull request Dec 19, 2017
Closed
Node.js Foundation Technical Steering Committee (TSC) Meeting 2017-12-20 #447

fixup: fix test for Windows

Loading status checks…

e7c8d07

targos closed this Jan 15, 2018

targos referenced this pull request Jan 15, 2018
Closed
src: delete process.env values set to undefined #18158
4 of 4 tasks complete

Trott removed the tsc-review label Jan 16, 2018

TimothyGu referenced this pull request Feb 25, 2018
Closed
process: doc-only deprecate non-string env value #18990
4 of 4 tasks complete

nodejs/node

Join GitHub today

child_process: ignore undef/proto values of env #15089

Conversation

Gerhut commented Aug 30, 2017 • edited

Checklist

Affected core subsystem(s)

nodejs-github-bot added the child_process label Aug 30, 2017

benjamingr reviewed Aug 30, 2017 View changes

This comment has been minimized.

benjamingr Aug 30, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

benjamingr commented Aug 30, 2017

benjamingr added the semver-major label Aug 30, 2017

This comment has been minimized.

benjamingr commented Aug 30, 2017

cjihrig reviewed Aug 30, 2017 View changes

cjihrig left a comment

This comment has been minimized.

addaleax commented Aug 31, 2017

This comment has been minimized.

BridgeAR commented Aug 31, 2017 • edited

Gerhut force-pushed the Gerhut:master branch from fa5df44 to ea03fc8 Aug 31, 2017

This comment has been minimized.

jasnell commented Sep 1, 2017

This comment has been minimized.

targos commented Sep 1, 2017

This comment has been minimized.

addaleax commented Sep 2, 2017

This comment has been minimized.

BridgeAR commented Sep 3, 2017

This comment has been minimized.

refack commented Sep 3, 2017

Gerhut force-pushed the Gerhut:master branch from ea03fc8 to 8c10ccd Sep 4, 2017

This comment has been minimized.

Gerhut commented Sep 4, 2017

BridgeAR reviewed Sep 11, 2017 View changes

BridgeAR left a comment

This comment has been minimized.

This comment has been minimized.

BridgeAR commented Sep 11, 2017

This comment has been minimized.

bnoordhuis commented Sep 11, 2017

Gerhut force-pushed the Gerhut:master branch from 8c10ccd to d3e7468 Sep 12, 2017

Gerhut changed the title child_process: ignore null/undefined values of env child_process: ignore undef/proto values of env Sep 12, 2017

jasnell approved these changes Sep 15, 2017 View changes

Gerhut force-pushed the Gerhut:master branch from d3e7468 to c3be4f7 Sep 18, 2017

BridgeAR dismissed their stale review Sep 19, 2017

This comment has been minimized.

BridgeAR commented Sep 22, 2017

This comment has been minimized.

evanlucas commented Sep 25, 2017

This comment has been minimized.

targos commented Sep 25, 2017

refack reviewed Sep 25, 2017 View changes

This comment has been minimized.

refack reviewed Sep 25, 2017 View changes

This comment has been minimized.

Gerhut force-pushed the Gerhut:master branch from c3be4f7 to cccd9d8 Sep 27, 2017

BridgeAR requested a review from nodejs/tsc Dec 6, 2017

addaleax approved these changes Dec 6, 2017 View changes

This comment has been minimized.

Gerhut force-pushed the Gerhut:master branch from e566e1b to 5e37647 Dec 6, 2017

This comment has been minimized.

Gerhut commented Dec 6, 2017

MylesBorins force-pushed the nodejs:master branch from b7405ab to 7f086dd Dec 8, 2017

rvagg referenced this pull request Dec 19, 2017

targos approved these changes Jan 15, 2018 View changes

This comment has been minimized.

targos commented Jan 15, 2018

This comment has been minimized.

targos commented Jan 15, 2018

Gerhut commented Aug 30, 2017 •

edited

benjamingr reviewed Aug 30, 2017

View changes

benjamingr Aug 30, 2017 •

edited

cjihrig reviewed Aug 30, 2017

View changes

BridgeAR commented Aug 31, 2017 •

edited

Gerhut force-pushed the Gerhut:master branch from `fa5df44` to `ea03fc8` Aug 31, 2017

Gerhut force-pushed the Gerhut:master branch from `ea03fc8` to `8c10ccd` Sep 4, 2017

BridgeAR reviewed Sep 11, 2017

View changes

Gerhut force-pushed the Gerhut:master branch from `8c10ccd` to `d3e7468` Sep 12, 2017

jasnell approved these changes Sep 15, 2017

View changes

Gerhut force-pushed the Gerhut:master branch from `d3e7468` to `c3be4f7` Sep 18, 2017

refack reviewed Sep 25, 2017

View changes

refack reviewed Sep 25, 2017

View changes

Gerhut force-pushed the Gerhut:master branch from `c3be4f7` to `cccd9d8` Sep 27, 2017

addaleax approved these changes Dec 6, 2017

View changes

Gerhut force-pushed the Gerhut:master branch from `e566e1b` to `5e37647` Dec 6, 2017

MylesBorins force-pushed the nodejs:master branch from `b7405ab` to `7f086dd` Dec 8, 2017

targos approved these changes Jan 15, 2018

View changes

Gerhut commented on `e7c8d07` Jan 15, 2018

MylesBorins commented Jan 17, 2018 •

edited