Defensive fix for process.config.variables regression (v5)

[sneak]

Updated defensive fix (v2 PR of #6110)

This is the 5.x PR, the 4.x one is here:

#6114

Discussion here:

#3755 (comment)

[jasnell]

The commit accidentally picked up the extraneous change to deps/npm/node_modules/request/node_modules/qs/README.md.

[sneak]

Yeah, I've no idea where that came from. I didn't git rm anything and haven't touched that file. Shall I recreate the PR?

[jasnell]

It's an artifact of another update that happened previously where git is not acting nicely with a casing change on the filename. It pops up from time to time, especially when switching between the v4.x-staging and v5.x branches. The easiest thing to do would be to do a hard reset of your dev branch to nodejs/v5.x (e.g. git checkout {your-branch-name} && git reset --hard {remote}/v5.x where {remote} is whatever you've named the upstream remote that points to nodejs/node). Then do a quick git status to make sure the qs/README.md doesn't show up as an unstaged change. If the git status comes up clean, reapply your fix to your branch and do a force push back up to your dev branch. (hopefully that makes sense ;-) ...)

[sneak]

done/done. (without long lines 😉)

[jasnell]

Thank you! CI: https://ci.nodejs.org/job/node-test-pull-request/2220/

[Fishrock123]

Why is this directly landing against release branches and not landing is master?

Also could we get some description as to how you do not have process.config.variables? To my understanding that will always exist, and if it doesn't it may be a build problem?

[jasnell]

@Fishrock123 ... the offending section of code does not exist in lib/_tls_wrap.js in master. My next step on this, btw, is to work up a repro test case to see if I can recreate it reliably (note that I hadn't given a LGTM on it yet ;-) ...)

[sneak]

I don't know what part of the test stack of mocha/coffeescript/istanbul/supertest is mucking about with it, but the app (which I didn't write and haven't modified in a few months) worked fine on 4.2.3 and stopped on 4.2.4. This makes it work again. Further deponent sayeth not. It breaks with the released binaries - I am not building it myself (or, wasn't, until I had to apply this patch).

The reason it's in the branches and not in master is because I haven't tested against master, and the part where the fix is applied doesn't seem to be be in master.

Look; I'm just a user. I didn't want to spend my entire Thursday tracking down why a test suite on unmodified code worked in October and doesn't work in April on the same LTS runtime; but here we are. This makes it better. Ideally, things (i.e. new features) like FIPS support wouldn't be pushed into an LTS branch at all in the first place (which in my understanding is generally limited to backporting security or logic fixes). I'm just trying to get my work done; learning how you guys build and release software in your neck of the woods is out of scope.

I can strip out code from the app in question to get it to a place I can share it, if you'd like.

The relevant section of package.json follows:

  "devDependencies": {
    "chai": "3.5.0",
    "istanbul": "0.4.3",
    "mocha": "2.4.5",
    "sqlite3": "^3.1.3",
    "supertest": "1.2.0",
    "supertest-as-promised": "3.1.0"
  },
  "dependencies": {
    "argparse": "1.0.2",
    "bitcoinjs-lib": "1.5.6",
    "body-parser": "1.13.2",
    "coffee-script": "1.10.0",
    "express": "4.13.1",
    "js-beautify": "1.5.10",
    "lodash": "^4.0.0",
    "moment": "2.10.6",
    "morgan": "1.6.1",
    "node-jsrender": "1.0.6",
    "nodemailer": "1.4.0",
    "nodemailer-smtp-transport": "1.0.3",
    "pg": "4.4.2",
    "pg-hstore": "2.3.2",
    "prompt-sync": "1.0.0",
    "q": "1.4.1",
    "sequelize": "3.17.0",
    "sequelize-cli": "1.9.1",
    "validator": "4.0.2",
    "winston": "2.2.0"
  }

[Fishrock123]

@sneak Could you console.log(process.config) from you app, and also do a grep (or similar search) for process.config?

[sneak]

I'm going to remove the proprietary stuff from the app to make a small reproducible test case I can share.

It's something one of the testing libraries is doing, I'm sure, to check test coverage or parallelize instances of the app to run tests faster. I'll get you a reproduction. I'm sure it's something stupid being done in a library, but that's no reason to not program defensively in /lib. Not sure why the change is controversial, considering it fixes the problem - but you'll have your test case momentarily.

[jasnell]

The change isn't controversial as far as I'm concerned but if it is something that one of the downstream modules is doing it would be good to know. (In general, we don't support cases where a module mucks around with Node.js internals). In this particular case, however, given that it's caused by the addition of the additional FIPS check, it's something we should get fixed and this update does exactly that.

[sneak]

Ahh, one of the test fixtures in the app was overwriting process.config with a new object (using it as a global config structure for the app during testing). I didn't catch it during review before because I didn't know runtime levers were writable like that.

I guess there was nothing important in there before the FIPS thing? The app passed all its tests for months.

[Fishrock123]

Maybe we should consider freezing some of those objects, if that doesn't give much of a perf hit?

[jasnell]

I was just typing the same thing. process.config could (and likely should) be made read only. Or, perhaps we turn process.config into a getter/setter and print a clear warning if a new value is set.

(It also looks like there's a slight error in the docs for process.config but that's not important here ;-) ...)

[jasnell]

Quick update on this: I'm working on getting #6266 landed in master. Once that's landed, I plan to backport it to v5 and v4 and refactor the parts inside lib/* that depend directly on process.config to avoid this issue entirely.

[MylesBorins]

@jasnell once that lands should we close this PR?

[jasnell]

No, let's keep this open until we can get the backport landed.

[MylesBorins]

As v5.x is now EOL I'm going to close this. Please feel free to let me know if this is not the right thing to do