GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — worker: support case-insensitive process.env usage on Windows in worker threads by daeyeon · Pull Request #48976 · nodejs/node

Conversation

@daeyeon

@daeyeon daeyeon commented Jul 31, 2023

Copy link
Copy Markdown
Member

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 [email protected]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss, debadree25, and pimterry reacted with thumbs up emoji ramoncerdaquiroz reacted with heart emoji
All reactions
  • 👍 3 reactions
  • ❤️ 1 reaction
@daeyeon
worker: support case-insensitive process.env usage on Windows
0cf71e2
This patch fixes the reported issue by using a case-insensitive
`unordered_map` for the copied `process.env` on Windows.

Signed-off-by: Daeyeon Jeong <[email protected]>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jul 31, 2023
StefanStojanovic
StefanStojanovic approved these changes Jul 31, 2023
View reviewed changes

@StefanStojanovic StefanStojanovic left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI passes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 31, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 31, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
anonrig
anonrig reviewed Aug 1, 2023
View reviewed changes
Comment thread src/node_env_var.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

@daeyeon
fixup! worker: support case-insensitive process.env usage on Windows
f373f7e
Signed-off-by: Daeyeon Jeong <[email protected]>
@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 1, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 1, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
bnoordhuis
bnoordhuis reviewed Aug 2, 2023
View reviewed changes

@bnoordhuis bnoordhuis left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/node_env_var.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

@daeyeon
fixup! fixup! worker: support case-insensitive process.env usage on W…
a955522
…indows

Signed-off-by: Daeyeon Jeong <[email protected]>
@daeyeon

daeyeon commented Aug 3, 2023

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 3, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 3, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Aug 3, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bnoordhuis

bnoordhuis commented Aug 3, 2023

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@daeyeon

daeyeon commented Aug 3, 2023

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

thefourtheye
thefourtheye reviewed Aug 3, 2023
View reviewed changes
Comment thread test/parallel/test-worker-process-env-windows.js
// Test for https://github.com/nodejs/node/issues/48955.

if (!common.isWindows) {
common.skip('this test is Windows-specific.');

@thefourtheye thefourtheye Aug 3, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@daeyeon daeyeon Aug 3, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

thefourtheye reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@daeyeon daeyeon mentioned this pull request Aug 3, 2023
doc: clarify use of process.env in worker threads on Windows #49008
Merged
@github-actions github-actions Bot mentioned this pull request Aug 4, 2023
CI Reliability 2023-08-04 nodejs/reliability#633
Open
25 tasks
@daeyeon

daeyeon commented Aug 6, 2023

Copy link
Copy Markdown
Member Author

Closing in favor of #49008

leumasme and pimterry reacted with confused emoji
All reactions
  • 😕 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@daeyeon daeyeon closed this Aug 6, 2023
@daeyeon
daeyeon deleted the main.worker-env-230731.Mon.9c94 branch August 6, 2023 02:07
@leumasme leumasme mentioned this pull request Aug 7, 2023
Terminal Intercepting crashes when using NodeJS worker threads httptoolkit/httptoolkit-server#91
Closed
@pimterry

pimterry commented Aug 7, 2023

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@daeyeon daeyeon mentioned this pull request Aug 8, 2023
In worker threads, process.env is case-sensitive on windows #48955
Closed
@daeyeon

daeyeon commented Aug 8, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@coderabbitai coderabbitai Bot mentioned this pull request Jun 16, 2026
feat(docker): Docker Services panel (containers, logs, shell, lifecycle, images/volumes/networks, connections) stablyai/orca#5522
Open
3 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments
@anonrig anonrig anonrig left review comments
@StefanStojanovic StefanStojanovic StefanStojanovic approved these changes
+1 more reviewer
@thefourtheye thefourtheye thefourtheye left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

In worker threads, process.env is case-sensitive on windows

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@daeyeon @nodejs-github-bot @bnoordhuis @pimterry @thefourtheye @anonrig @StefanStojanovic
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.