-
Notifications
You must be signed in to change notification settings - Fork 16.9k
fix: don't run environment bootstrapper #22342
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
Conversation
c33e8c4 to
31fe815
Compare
nornagon
left a comment
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.
Seems like a nice simplification. Do you think it would be worth adding a test to prevent regression?
|
@nornagon oh good reminder - there are a few Node.js tests we had disabled that were actually commented out because of the undesirable behavior. I'll go re-add those and that should serve as a regression test! |
b01ca9d to
02dff2e
Compare
02dff2e to
7564a60
Compare
|
Release Notes Persisted
|
|
I was unable to backport this PR to "9-x-y" cleanly; |
|
I was unable to backport this PR to "8-x-y" cleanly; |
|
I was unable to backport this PR to "9-x-y" cleanly; |
|
I was unable to backport this PR to "8-x-y" cleanly; |
2 similar comments
|
I was unable to backport this PR to "8-x-y" cleanly; |
|
I was unable to backport this PR to "8-x-y" cleanly; |
Description of Change
Closes #21907.
In nodejs/node#26788 (v12.0.0), Node.js added a bootstrapper to
CreateEnvironment, which would prepare the main thread for execution any time an embedder created a new environment. However, this caused an unfortunate doubling-up effect; Node.js also ran bootstrapping (calledprepareMainThreadExecution) for all other execution paths (like the repl, the actual main module, eval, etc). This meant that we'd see weird stuff likesince both execution pathways (env and whatever the actual execution was) were being exercised. To fix this, we can just remove bootstrapping code from
CreateEnvironment. This also saves us a bunch of workaround code on our end, like the unsafe env block we had to prevent strange behavior during debugger setup. An upstream PR is open for this against Node.js itself, linked at bottom.cc @MarshallOfSound @zcbenz @nornagon
Checklist
npm testpassesRelease Notes
Notes: Fixed an issue where some logging would double-print.