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.

Sign up for GitHub

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

Jump to bottom

lib: avoid mutating Error.stackTraceLimit when it is not writable #38215

Merged
merged 1 commit into from
Apr 14, 2021
Merged

lib: avoid mutating Error.stackTraceLimit when it is not writable #38215

merged 1 commit into from
Apr 14, 2021

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Apr 12, 2021

This PR aims to avoid getting Cannot assign to read only property 'stackTraceLimit' of function 'function Error() { [native code] }' errors thrown from core when Node.js is executed with the --frozen-intrinsics flag.

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Apr 12, 2021
@aduh95 aduh95 mentioned this pull request Apr 12, 2021
Merged
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 12, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/37405/

mscdex
mscdex reviewed Apr 12, 2021
View reviewed changes
lib/assert.js Outdated Show resolved Hide resolved
bmeck
bmeck suggested changes Apr 12, 2021
View reviewed changes
Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realized this should be checking configurable, not frozen status to handle if someone does an Object.seal(Error), etc.

@aduh95 aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 12, 2021
@aduh95
Copy link
Contributor Author

aduh95 commented Apr 13, 2021 •
edited
Loading

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/998/

Results look OK-ish:

                                                               confidence improvement accuracy (*)    (**)   (***)
misc/hidestackframes.js n=100000 type='direct-call-noerr'                      0.02 %       ±5.94%  ±7.91% ±10.30%
misc/hidestackframes.js n=100000 type='direct-call-throw'               *     -2.86 %       ±2.36%  ±3.14%  ±4.09%
misc/hidestackframes.js n=100000 type='hide-stackframes-noerr'                 0.34 %      ±15.71% ±20.91% ±27.25%
misc/hidestackframes.js n=100000 type='hide-stackframes-throw'                -2.86 %       ±3.23%  ±4.30%  ±5.60%

@aduh95 aduh95 changed the title lib: avoid mutating Error.stackTraceLimit when Error is frozen lib: avoid mutating Error.stackTraceLimit when it is not writable Apr 13, 2021
@aduh95 aduh95 requested a review from bmeck April 13, 2021 10:44
aduh95
aduh95 commented Apr 13, 2021
View reviewed changes
lib/internal/errors.js Outdated
return ObjectIsExtensible(Error);
}

return desc.writable ?? typeof desc.set === 'function';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could try to cache the value when the property is not configurable:

Suggested change
return desc.writable ?? typeof desc.set === 'function';
const returnValue = desc.writable ?? typeof desc.set === 'function';
if (desc.configurable === false)
module.exports.isErrorStackTraceLimitWritable = () => returnValue;
return returnValue;

I think it makes the harder to read, and I think we should try to optimize for the most common case (which is when the property has configurable and writable set to true).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine not cacheing for now

bmeck
bmeck reviewed Apr 13, 2021
View reviewed changes
lib/internal/errors.js Outdated Show resolved Hide resolved
@aduh95 aduh95 requested a review from bmeck April 13, 2021 14:27
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/37440/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/37441/

@aduh95
Copy link
Contributor Author

aduh95 commented Apr 13, 2021 •
edited
Loading

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/999/

EDIT: the results are not as good… I'd say the tradeoff is worth it, I'm open to suggestions to improve those figures.

                                                               confidence improvement accuracy (*)    (**)   (***)
misc/hidestackframes.js n=100000 type='direct-call-noerr'                      2.68 %       ±4.40%  ±5.86%  ±7.62%
misc/hidestackframes.js n=100000 type='direct-call-throw'              **     -4.20 %       ±3.04%  ±4.05%  ±5.29%
misc/hidestackframes.js n=100000 type='hide-stackframes-noerr'                13.07 %      ±14.03% ±18.86% ±24.95%
misc/hidestackframes.js n=100000 type='hide-stackframes-throw'        ***     -4.70 %       ±2.42%  ±3.22%  ±4.19%

ExE-Boss
ExE-Boss reviewed Apr 13, 2021
View reviewed changes
lib/internal/errors.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/37457/

@aduh95
Copy link
Contributor Author

aduh95 commented Apr 14, 2021

I've added more tests. I think this is ready to land but I'd love to get a final approval to make sure I haven't forgotten anything :)

bmeck
bmeck approved these changes Apr 14, 2021
View reviewed changes
Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 14, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/37465/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/37472/

@aduh95
lib: avoid mutating Error.stackTraceLimit when it is not writable
09c9e5d 
PR-URL: nodejs#38215
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@aduh95
Copy link
Contributor Author

aduh95 commented Apr 14, 2021

Landed in 09c9e5d

@aduh95 aduh95 merged commit 09c9e5d into nodejs:master Apr 14, 2021
@aduh95 aduh95 deleted the frozen-error branch April 14, 2021 22:18
BethGriggs pushed a commit that referenced this pull request Apr 15, 2021
@aduh95 @BethGriggs
lib: avoid mutating Error.stackTraceLimit when it is not writable
dfdf348 
PR-URL: #38215
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Apr 16, 2021
Merged
@liuxingbaoyu liuxingbaoyu mentioned this pull request May 13, 2023
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@ExE-Boss ExE-Boss ExE-Boss left review comments

@bmeck bmeck bmeck approved these changes

@benjamingr benjamingr benjamingr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@aduh95 @nodejs-github-bot @mscdex @bmeck @benjamingr @ExE-Boss @targos