Skip to content
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

fix: perform microtask checkpoint before MicrotaskRunner destruction #38828

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ckerr
Copy link
Member

@ckerr ckerr commented Jun 17, 2023

Description of Change

Fix a bug that caused periodic test failures. Sample failure:

not ok 396 utilityProcess module stderr property is valid when child process launches with pipe stdio configuration
expected '' to equal 'world'
AssertionError: expected '' to equal 'world'
at Context. (electron/spec/api-utility-process-spec.ts:176:22)
at processTicksAndRejections (node:internal/process/task_queues:95:5)

The problem seems to be that the test spawned this script which wrote to stderr immediately before exiting and it was possible for there to still be pending queued tasks when NodeService destroyed the microtask runner.

The approach extracts the tell-v8-to-perform-a-checkpoint code from MicrotaskRunner::DidProcessTask() into a new public convenience function MicrotaskRunner::PerformCheckpoint(). It calls that function from JavascriptEnvironment::DestroyMicrotaskRunner() before destroying the microtask runner.

In main I'm able to trigger this test failure about 50% of the time. In this branch I wasn't able to break it in 100 iterations.

CC @deepak1556 who has done a lot of work in this code in #24131 and #34980 and @codebytere's CallbackScope code in #27001

Checklist

Release Notes

Notes: Fixed bug that could fail to process pending microtasks added to a child process immediately before calling process.exit().

@ckerr ckerr added semver/patch backwards-compatible bug fixes 24-x-y 25-x-y 26-x-y labels Jun 17, 2023
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 17, 2023
@ckerr ckerr changed the title Fix/test flake stderr property is valid when child process launches with pipe stdio configuration fix: pending-microtasks-on-exit bug that caused stderr test flake Jun 17, 2023
@ckerr ckerr marked this pull request as draft June 17, 2023 04:43
@ckerr
Copy link
Member Author

ckerr commented Jun 17, 2023

Looks like this change needs some work; it introduces a regression in

utilityProcess module lifecycle events emits 'exit' when there is uncaught exception

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 18, 2023
@codebytere codebytere force-pushed the fix/test-flake--stderr-property-is-valid-when-child-process-launches-with-pipe-stdio-configuration branch from 4172fca to cf48ad7 Compare October 3, 2023 15:47
@codebytere codebytere changed the title fix: pending-microtasks-on-exit bug that caused stderr test flake fix: perform microtask checkpoint before MicrotaskRunner destruction Oct 3, 2023
@codebytere codebytere added target/25-x-y PR should also be added to the "25-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. target/27-x-y PR should also be added to the "27-x-y" branch. and removed 24-x-y 25-x-y 26-x-y labels Oct 3, 2023
@codebytere codebytere marked this pull request as ready for review October 3, 2023 17:13
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 3, 2023
@codebytere codebytere requested review from deepak1556, MarshallOfSound, RaisinTen and VerteDinde and removed request for deepak1556 October 3, 2023 17:14
@deepak1556
Copy link
Member

I like the change in this PR, however I am wondering if the following was also considered. Currently we drain pending macro tasks on the uv loop when the JavaScriptEnvironment is destroyed

platform_->DrainTasks(isolate_);
, if we did it before the microtask runner is destroyed it should trigger the checkpoints instead of us having to trigger it manually.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 4, 2023
@codebytere
Copy link
Member

@deepak1556 there are some contexts where we don't have a MicrotaskRunner (for ex. NodeMain) - are you suggesting we move the DrainTasks call out of the dtor or just duplicate it into JavascriptEnvironment::DestroyMicrotasksRunner()?

@deepak1556
Copy link
Member

deepak1556 commented Oct 5, 2023

I am suggesting to move DrainTasks out of the dtor and call it before destorying the MicrotasksRunner in the browser and utility process, for other process move it to relevant places. Another reason to move this out would be, currently we don't terminate isolate during process exit so if the tasks were to enter JS we don't stop it from happening and having them run after environment destruction seems incorrect. I am also considering to mark the isolate for termination in the utility process due to a recent bug I started looking at.

@github-actions github-actions bot added the target/28-x-y PR should also be added to the "28-x-y" branch. label Oct 11, 2023
@github-actions github-actions bot added the target/29-x-y PR should also be added to the "29-x-y" branch. label Dec 6, 2023
@github-actions github-actions bot added the target/30-x-y PR should also be added to the "30-x-y" branch. label Feb 21, 2024
@miniak miniak removed target/25-x-y PR should also be added to the "25-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. target/27-x-y PR should also be added to the "27-x-y" branch. labels Apr 15, 2024
@github-actions github-actions bot added the target/31-x-y PR should also be added to the "31-x-y" branch. label Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch backwards-compatible bug fixes target/28-x-y PR should also be added to the "28-x-y" branch. target/29-x-y PR should also be added to the "29-x-y" branch. target/30-x-y PR should also be added to the "30-x-y" branch. target/31-x-y PR should also be added to the "31-x-y" branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants