GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||
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
Member
Fixes: #48955 By default, a Worker thread uses a copy of parent thread's This fixes the issue by using a case-insensitive Signed-off-by: Daeyeon Jeong [email protected]
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
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]>
StefanStojanovic
approved these changes
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
This comment was marked as outdated.
anonrig
reviewed
Uh oh!
There was an error while loading. Please reload this page. Signed-off-by: Daeyeon Jeong <[email protected]>
This comment was marked as outdated.
bnoordhuis
reviewed
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 reactionsUh oh!
There was an error while loading. Please reload this page. …indows Signed-off-by: Daeyeon Jeong <[email protected]>
Member
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as outdated.
Collaborator
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Member
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
thefourtheye
reviewed
|
|
Closing in favor of #49008 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
@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:
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
In worker threads, process.env is case-sensitive on windows
There was an error while loading. Please reload this page.