Skip to content

fix: don't run environment bootstrapper - #22342

Merged
codebytere merged 2 commits into
masterfrom
fix-double-warn
Feb 25, 2020
Merged

codebytere merged 2 commits into
masterfrom
fix-double-warn

Conversation

@codebytere

@codebytere codebytere commented Feb 22, 2020

Copy link
Copy Markdown
Member

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

@nornagon nornagon left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

release-clerk Bot commented Feb 25, 2020

Copy link
Copy Markdown

Release Notes Persisted

Fixed an issue where some logging would double-print.

@trop

trop Bot commented Feb 25, 2020

Copy link
Copy Markdown
Contributor

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

@trop

trop Bot commented Feb 25, 2020

Copy link
Copy Markdown
Contributor

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

@trop

trop Bot commented Feb 25, 2020

Copy link
Copy Markdown
Contributor

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

@trop

trop Bot commented Feb 25, 2020

Copy link
Copy Markdown
Contributor

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

2 similar comments
@trop

trop Bot commented Feb 25, 2020

Copy link
Copy Markdown
Contributor

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

@trop

trop Bot commented Feb 25, 2020

Copy link
Copy Markdown
Contributor

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

@trop

trop Bot commented May 21, 2020

Copy link
Copy Markdown
Contributor

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

@trop

trop Bot commented Jun 2, 2020

Copy link
Copy Markdown
Contributor

@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

3 participants

Sponsor
SponsoredKunjungi sekarang
Promo