Skip to content

Conversation

@codebytere
Copy link
Member

@codebytere codebytere commented Feb 22, 2020

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 (called prepareMainThreadExecution) for all other execution paths (like the repl, the actual main module, eval, etc). This meant that we'd see weird stuff like

> process.emit('warning', new Error('warn'))
(node:55797) Error: warn
(node:55797) Error: warn

since 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

Release Notes

Notes: Fixed an issue where some logging would double-print.

@codebytere codebytere requested a review from a team as a code owner February 22, 2020 06:54
@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Feb 22, 2020
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Feb 23, 2020
@codebytere codebytere requested a review from zcbenz February 24, 2020 03:07
Copy link
Contributor

@nornagon nornagon left a 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?

@codebytere
Copy link
Member Author

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

@codebytere codebytere changed the title fix: dont run environment bootstrapper fix: don't run environment bootstrapper Feb 24, 2020
@codebytere codebytere force-pushed the fix-double-warn branch 3 times, most recently from b01ca9d to 02dff2e Compare February 25, 2020 03:09
@codebytere codebytere merged commit 79270e3 into master Feb 25, 2020
@codebytere codebytere deleted the fix-double-warn branch February 25, 2020 16:46
@release-clerk
Copy link

release-clerk bot commented Feb 25, 2020

Release Notes Persisted

Fixed an issue where some logging would double-print.

@trop
Copy link
Contributor

trop bot commented Feb 25, 2020

I was unable to backport this PR to "9-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Feb 25, 2020

I was unable to backport this PR to "8-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Feb 25, 2020

I was unable to backport this PR to "9-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Feb 25, 2020

I was unable to backport this PR to "8-x-y" cleanly;
you will need to perform this backport manually.

2 similar comments
@trop
Copy link
Contributor

trop bot commented Feb 25, 2020

I was unable to backport this PR to "8-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Feb 25, 2020

I was unable to backport this PR to "8-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented May 21, 2020

@ckerr has manually backported this PR to "9-x-y", please check out #23689

@trop
Copy link
Contributor

trop bot commented Jun 2, 2020

@ckerr has manually backported this PR to "8-x-y", please check out #23924

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

'warn' event being handled twice

4 participants