-
Notifications
You must be signed in to change notification settings - Fork 3.8k
fix(worker_threads): Accept SHARE_ENV symbol in worker env option #21877
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Fixes validation error when using worker_threads.SHARE_ENV as the env option in Worker constructor. The validation logic now properly recognizes the SHARE_ENV symbol and allows it to pass through, implementing the intended behavior of sharing environment variables from the parent process. Fixes #20451 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…patibility - Use Symbol.for() instead of Symbol() to match Node.js exactly - Fix environment sharing logic to enable true parent environment access - Add comprehensive tests for SHARE_ENV behavior including bidirectional sharing - Ensure SHARE_ENV allows workers to see dynamically set parent env variables The implementation now correctly follows Node.js semantics where SHARE_ENV enables true environment sharing rather than just copying static environment. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
Updated 8:27 PM PT - Aug 14th, 2025
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 21877That installs a local version of the PR into your bun-21877 --bun |
| auto* symbol = asSymbol(envValue); | ||
| // Check if this is the global symbol for nodejs.worker_threads.SHARE_ENV | ||
| auto description = symbol->description(); | ||
| if (!description.isEmpty() && description == "nodejs.worker_threads.SHARE_ENV"_s) { |
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.
This is not correct. It needs to use symbolRegistry()->keyFor or something along those lines. It needs to use the sybmol registry and check the two Symbol's are equal and not the description value
Address PR feedback by replacing symbol description comparison with proper symbol registry comparison. This ensures correct Symbol.for() semantics and follows JSC best practices. - Use vm.symbolRegistry().symbolForKey() to get the expected symbol - Compare Symbol objects directly instead of descriptions - More robust and matches Node.js implementation approach 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
✅ Feedback addressed! Updated the implementation to use symbol registry comparison as suggested:
The fix now correctly handles Thanks for the excellent feedback! 🙏 |
Summary
Fixes validation error when using
worker_threads.SHARE_ENVas the env option in Worker constructor. The validation logic now properly recognizes the SHARE_ENV symbol and allows it to pass through, implementing the intended behavior of sharing environment variables from the parent process.Changes
SHARE_ENVsymbol in C++ validation codeSymbol.for()instead ofSymbol()to match Node.js exactlyNode.js Compatibility
The implementation now correctly follows Node.js semantics:
Symbol.for("nodejs.worker_threads.SHARE_ENV"))Test Plan
Before
After
Fixes #20451
🤖 Generated with Claude Code