test: async context for thrown microtasks - #64147
Conversation
nodejs-github-bot
added
needs-ci
|
cc @nodejs/diagnostics |
There was a problem hiding this comment.
Don't we generally want the context to be available in uncaughtException? We did a bunch of work at one point to ensure it would be there so uncaughtException handlers could attribute the errors to spans they came from in tracers. It'd probably be more correct to cut off the context after those handlers run. 🤔
I thought as well. The key assertion here is this one: https://github.com/nodejs/node/pull/64147/changes#diff-6c14bb576b8a1eaf12aeb421aec5efd35b58d92c8a02d668a83a2f74f9c6474fR18. |
|
Note that if we think the current behavior is correct, then I'll add a test for it to match. |
ronag
added
request-ci
github-actions
Bot
removed
the
request-ci
|
@mcollina The commit needs to be signed. |
Signed-off-by: Matteo Collina <hello@matteocollina.com>
f091a7a to
f2da5c5
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #64147 +/- ##
==========================================
- Coverage 90.32% 90.14% -0.19%
==========================================
Files 759 751 -8
Lines 248342 253648 +5306
Branches 46866 47774 +908
==========================================
+ Hits 224320 228648 +4328
- Misses 15461 16252 +791
- Partials 8561 8748 +187 🚀 New features to boost your workflow:
|
|
@trivikr PTAL |
legendecas
removed
the
author ready
|
I agree the behavior as described by @Qard at #64147 (review) is expected, that an uncaughtException could observe the context where it's uncaught. Could we add a test on the current behavior instead? |
| assert.strictEqual(asyncLocalStorage.getStore(), undefined); | ||
| })); | ||
|
|
||
| asyncLocalStorage.run(sensitive, () => { |
There was a problem hiding this comment.
The test illustrates a secretive AsyncLocalStorage store. But to actually expose an AsyncLocalStorage value, it has to be exported programmatically, i.e. either export the AsyncLocalStorage instance, or export a value getter. So I don't find this context being available at uncaughtException problematic.
The AsyncLocalStorage context of a throwing microtask remains current while the exception is being reported: the uncaughtException handler, exception formatting, and microtasks queued during formatting all observe it. Add a regression test documenting this current behavior. Signed-off-by: Matteo Collina <hello@matteocollina.com>
mcollina
changed the title
6 participants
Documents the current AsyncLocalStorage behavior when a microtask throws: the throwing microtask's context frame remains current while the exception is being reported, so the
uncaughtExceptionhandler, exception formatting, and microtasks queued during formatting all observe it.Adds a regression test covering this behavior with a non-Error object thrown from a microtask whose string conversion queues another microtask.