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

src: prevent hard coding stack trace limit #30752

Closed
wants to merge 1 commit into from

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Dec 1, 2019

Refer to Environment::stack_trace_limit() while printing fresh
stacktraces in c++ land.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 1, 2019
@legendecas legendecas mentioned this pull request Dec 1, 2019
4 tasks
@devsnek
Copy link
Member

devsnek commented Dec 1, 2019

the number of frames here should already be the <= stack trace limit. as I understand it our code just provides a lower bound.

@joyeecheung
Copy link
Member

joyeecheung commented Dec 1, 2019

TBH this does not sound like a good idea since v8 is considering removing these non-standard APIs: https://bugs.chromium.org/p/v8/issues/detail?id=6974 By adding support in our internal error printers we are sort of encouraging people to use it - also in the case of --trace-sync-io, it is more of an implementation detail that we use the error-related API to print things.

It is especially tricky to try handling these hooks ourselves considering we also made the mistake (?) of always respecting the Error.prepareStackTrace from the main context.

src/node_errors.cc Outdated Show resolved Hide resolved
src/node_errors.cc Outdated Show resolved Hide resolved
src/node_errors.cc Outdated Show resolved Hide resolved
src/node_errors.cc Outdated Show resolved Hide resolved
src/node_errors.cc Outdated Show resolved Hide resolved
MaybeLocal<Value> stack_trace_limit_val =
context->Global()
->Get(context, error_string)
.ToLocalChecked()
Copy link
Member

Choose a reason for hiding this comment

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

This is not a valid .ToLocalChecked(), can you handle errors here (and for the .ToChecked() below, as @lundibundi pointed out)? Ideally, this block here would also be wrapped in a v8::TryCatch in order to swallow these errors from trying to get the stack trace

@legendecas
Copy link
Member Author

@joyeecheung Yes, the API Error.stackTraceLimit itself is not standard. The idea of PR is preventing hard-coding numbers in the codebase. Either the limit shall be referred from the global Error.stackTraceLimit or just stored in the node::Environment, it would sound good to me. Since we've already used Error.stackTraceLimit multiple places, I'd think it might be fine at the time. We could remove the usage on Error.stackTraceLimit later if it's time to deprecate the API.

Anyway, if anyone has any strong opinion on this idea, it also sounds good to me if the stacktrace limit shall be referred from some internal store.

@joyeecheung
Copy link
Member

@legendecas Adding something like Environment::kStackTraceLimit SGTM. I think most internal uses of Error.stackTraceLimit are only optimizations to avoid the overhead of generating a stack trace (with the pattern of setting it to 0 or 1, creating an error, and then restoring the limit), which I don't know if is actually effective at this point considering stack trace generation in v8 is lazy.

@legendecas legendecas force-pushed the cpp-error-stack-limit branch 5 times, most recently from 9bf92aa to e6f54bb Compare December 7, 2019 07:23
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

I guess LGTM although I’d have preferred the solution that reads Error.stackTraceLimit

@legendecas
Copy link
Member Author

@joyeecheung @lundibundi may I ask for your reviews on the PR since you have interests in preventing usage on the unstandardized Error.stackTraceLimit?

src/env.h Outdated Show resolved Hide resolved
Copy link
Member

@lundibundi lundibundi left a comment

Choose a reason for hiding this comment

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

LGTM. Though I would prefer the previous Error.stackTraceLimit version as it is useful to be able to control the C++-land stack size and to have it consistent with the JS-land. Regarding possible removal of the API, I think it is not much of an issue as this code will only be in once place and we can always replace it with a constant in a few days' time at max.

Also, the PR's name should be changed accordingly to the code.

src/node_errors.cc Show resolved Hide resolved
@legendecas
Copy link
Member Author

this code will only be in once place

There are some to be landed recently or in an expected future. #29207 and #30516. I'll update the code accordingly.

@legendecas
Copy link
Member Author

@devsnek the number of frames here should already be the <= stack trace limit. as I understand it our code just provides a lower bound.

It's true that in most cases stack trace wasn't very long to exceed the limit. Still, it is possible.

@lundibundi
Copy link
Member

this code will only be in once place

There are some to be landed recently or in an expected future. #29207 and #30516. I'll update the code accordingly.

I meant to have one function like get_stack_size or compute_stack_size and then use it everywhere. Then we can easily just make this function return 10; if needed.

@legendecas legendecas changed the title src: respect Error.stackTraceLimit in c++ land src: prevent hard coding stack trace limit Dec 19, 2019
@legendecas legendecas force-pushed the cpp-error-stack-limit branch 2 times, most recently from 543418f to 4f1f62a Compare December 19, 2019 15:23
Refer to Environment::stack_trace_limit() while printing fresh
stacktraces in c++ land.
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR
Copy link
Member

I agree with @addaleax and @lundibundi that the former solution seemed better. If the property will ever be removed, it's definitely possible to react to it appropriately. So far it's not likely that this is happening anytime soon.

@joyeecheung
Copy link
Member

joyeecheung commented Dec 24, 2019

I would not oppose to a solution that allows the JS land to configure the stack trace limit, but I would be -1 if this is Error.stackTraceLimit for the following reasons (other than it's non-standard):

  1. It's an implementation detail that we use the error stack trace API to print stack traces that are not necessarily related to errors (e.g. --trace-sync-io) (actually, I don't think we currently even use the error stack trace API at all in these cases, so we'd be adding dependencies instead)
  2. It's unsafe to access Error.stackTraceLimit (which can be an accessor) in some of the paths where stack traces are printed from C++ (e.g. in Environment::Exit)

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 25, 2019
@nodejs-github-bot
Copy link
Collaborator

BridgeAR pushed a commit that referenced this pull request Dec 25, 2019
Refer to Environment::stack_trace_limit() while printing fresh
stacktraces in c++ land.

PR-URL: #30752
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR
Copy link
Member

Landed in 8baee5e 🎉

@BridgeAR BridgeAR closed this Dec 25, 2019
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
Refer to Environment::stack_trace_limit() while printing fresh
stacktraces in c++ land.

PR-URL: #30752
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@legendecas legendecas deleted the cpp-error-stack-limit branch January 5, 2020 05:37
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
targos pushed a commit that referenced this pull request Jan 14, 2020
Refer to Environment::stack_trace_limit() while printing fresh
stacktraces in c++ land.

PR-URL: #30752
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2020
Refer to Environment::stack_trace_limit() while printing fresh
stacktraces in c++ land.

PR-URL: #30752
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Refer to Environment::stack_trace_limit() while printing fresh
stacktraces in c++ land.

PR-URL: #30752
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants