worker: support case-insensitive process.env usage on Windows in worker threads

[daeyeon]

Fixes: #48955

By default, a Worker thread uses a copy of parent thread's process.env, which is currently case-sensitive. It can be a problem on Windows using case-insensitive env keys, as reported issue.

This fixes the issue by using a case-insensitive unordered_map for the copied process.env on Windows.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

[StefanStojanovic]

LGTM if CI passes.

[bnoordhuis]

I'm sorta -.5 on this pull request. Yes, real environment variables are case-insensitive on Windows but deliberately mimicking that behavior for fake environment variables feels kind of yuck.

I expect it'll end up confusing people coding on Windows but deploying on Linux.

[daeyeon]

I understand the concern. Any mimicking could always break where we least expect it. And I think this issue also come from such a mimicking based on copied environment variables.

In this case, do you think it would be better to document the limitation of the copy? Alternatively, if you have any other suggestions, I would be glad to hear them.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/53055/

[bnoordhuis]

do you think it would be better to document the limitation of the copy?

You may want to get opinions from more people but that would be my suggestion, yes.

[daeyeon]

I'm actually leaning towards that way too now. Will open another PR.

[thefourtheye]

Just wondering if it would make sense to modify this test for non-windows also to validate if the case-sensitivity is honoured.

[daeyeon]

Yes, it makes sense. That could be like:

const { isWindows } = require('../common');

if (isMainThread) {
  process.env.barkey = 'foovalue';

  strictEqual(process.env.BARKEY, isWindows ? process.env.barkey : undefined);

  const worker = new Worker(__filename);
  worker.once('message', (data) => {
    strictEqual(data.BARKEY, isWindows ? data.barkey : undefined);
  });
} else {
  parentPort.postMessage({
    barkey: process.env.barkey,
    BARKEY: process.env.BARKEY,
  });
}

I would follow up on this PR based on feedback regarding the document fix. Thanks.

[daeyeon]

Closing in favor of #49008

[pimterry]

I'm sorta -.5 on this pull request. Yes, real environment variables are case-insensitive on Windows but deliberately mimicking that behavior for fake environment variables feels kind of yuck.

I expect it'll end up confusing people coding on Windows but deploying on Linux.

@bnoordhuis @daeyeon As a counterpoint: this current behaviour causes problems that are very confusing when coding on Linux but deploying on Windows (and I have hit issues related to this specifically in production).

Similarly, it will cause confusion when just coding on Windows normally, regardless of deployments, when correct & worker-compatible code inexplicably works differently just because it was moved from the main thread into a worker.

Is it possible to reconsider closing this?

A motivating example:

• Launch a Node process, with both HTTP_PROXY and http_proxy env vars set to the same value (useful for Linux, where these are different values, and in quite a few weird cases apps will prefer one or the other).
• On Linux, reading HTTP_PROXY or http_proxy in the Node process will always work, as expected - great.
• On Windows, due to case insensitivity, those collapse into a single environment variable.
• On Windows in the main thread, reading both variables works just the same as on Linux - great.
• On Windows within a worker thread, only the http_proxy lowercase env var is now set and the uppercase env var has mysteriously disappeared, due to this issue. Even though the application was launched with HTTP_PROXY set explicitly, that value is gone in the worker thread - very bad.

Effectively unsetting environment variables unexpectedly could cause all sorts of serious problems, and is definitely confusing, even if it's documented. I wouldn't be surprised if the inconsistency more generally caused other weird edge cases.

It would be much better to fix this so env vars on Windows always work the same everywhere, rather than just documenting the behaviour as broken in workers.

[daeyeon]

@leumasme @pimterry Apologies, I assumed there were no objections to addressing issue by the documentation. I should have pinged before closing it. Reopened the issue instead of this PR.