Uh oh!
There was an error while loading. Please reload this page.
errors: remove source map rekey logic - #39025
Conversation
If an error is thrown before a module is loaded, we attempt to cache source map against error object, rather than module object. We can't do this if the error is a primitive type Fixesnodejs#38945
| // from the module cache and our WeakMap. To allow a source map to still be | ||
| // applied to the stack trace when this happens, we rekey on the thrown error. | ||
| // This is only possible if the error is an object and not a primitive type. | ||
| if (sourceMap && typeof newInstance === 'object') { |
There was a problem hiding this comment.
Could newInstance be null, and if so, wouldn't that cause an error? Can you add a test case please?
There was a problem hiding this comment.
@aduh95 I did some testing it no longer seems like that rekey logic is needed to ensure that source maps are written to disk prior to exit. Perhaps this is because of refactors that have taken place related to source map serialization? Or perhaps a FinalizationRegistry fires on the next tick, such that the source map will be available while the process shuts down?
Either way, no tests fail if we simply remove the rekey logic.
There was a problem hiding this comment.
Nice! I still think it'd be interesting to have a test case with a throw null for completeness sake.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Rich Trott <rtrott@gmail.com>
bcoe
commented
Jun 13, 2021
@addaleax I realized while addressing @aduh95's feedback that the test suite seems to work currently without the rekeying logic. I tested against the c8 test suite too, and it passes after removing rekey. This logic originally existed for the edge case of an exception being thrown while a module was being loaded, which would mean that it was already garbage collected before source maps were serialized to disk, or applied to stack traces. I'm a little surprised tests now pass without the logic, but my temptation is to remove the code if it's not needed ... perhaps the GC of WeakMap values is doesn't happen before the process exits? |
nodejs-github-bot
commented
Jun 14, 2021
@bcoe Do you think this is backportable to v14.x? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
nodejs-github-bot
commented
Jun 15, 2021
bcoe
commented
Jun 15, 2021
@aduh95 I don't see any reason why not. |
If an error is thrown before a module is loaded, we attempt to cache source map against error object, rather than module object. We can't do this if the error is a primitive type Fixes#38945 PR-URL: #39025 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
bcoe
commented
Jun 22, 2021
Landed in c5cc3d4 |
If an error is thrown before a module is loaded, we attempt to cache source map against error object, rather than module object. We can't do this if the error is a primitive type Fixes#38945 PR-URL: #39025 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
If an error is thrown before a module is loaded, we attempt to cache source map against error object, rather than module object. We can't do this if the error is a primitive type Fixes#38945 PR-URL: #39025 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
If an error is thrown before a module is loaded, we attempt to cache source map against error object, rather than module object. We can't do this if the error is a primitive type Fixes#38945 PR-URL: #39025 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
The rekeying logic no longer seems to be necessary to ensure that source maps
are written to disk before the application exits.
Fixes#38945
I believe improvements could be made to both this class of error and to the error described in #39017, such that we try to still annotate the stack trace with the original call site. But, in both cases, I think it's good to land the obvious fix initially.
What we ultimately lose with source-maps enabled is the syntactic sugar of:
Which I don't think is standard stack trace output any ways.