-
-
Notifications
You must be signed in to change notification settings - Fork 34k
process: preserve AsyncLocalStorage on queueMicrotask only when needed #60913
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
ae950e3 to
a6615a8
Compare
a6615a8 to
2f8776e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #60913 +/- ##
==========================================
+ Coverage 88.55% 88.58% +0.02%
==========================================
Files 703 703
Lines 208291 208296 +5
Branches 40170 40166 -4
==========================================
+ Hits 184443 184509 +66
+ Misses 15874 15807 -67
- Partials 7974 7980 +6
🚀 New features to boost your workflow:
|
|
cc @nodejs/performance WDYT? |
|
Before merging, some concerns raised in another PR with same changes:
Would mind giving some thoughts on this one, @mcollina and @Qard? Edit: |
Failed to start CI⚠ No approving reviews found ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/19980375054 |
|
@H4ad Yes, the same concern applies. It is, in my opinion, problematic that it was ever possible to start seeing events for a given task partway through its lifecycle rather than deciding at emit time if any of the events should be present. But that is certainly a breaking change and so should be marked as such. A desirable change, I think. But a breaking one nonetheless. At the same time though, async_hooks is still marked as experimental, and AsyncLocalStorage no longer relies on it, so the break is likely a lot less critical. We may be able to allow it without a major bump. I'm not strongly opinionated either way. |
mcollina
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.
lgtm
|
I'm ok in this being a minor, but I don't think we can backport it to 22. |
Does the same optimization as #59873 for
queueMicrotaskbranch:
main