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

fix(node): ignore error rethrows in local variables integration #13418

Conversation

Bruno-DaSilva
Copy link

@Bruno-DaSilva Bruno-DaSilva commented Aug 18, 2024

Fixes #13416.

We ignore debugger call stacks in the worker that don't match the original error's call stack.

This should happen in just one of two ways, from my knowledge:

  1. a thrown error is caught and re-thrown, in which case we don't want the rethrow to override the error's variables from the original throw
  2. an error is created via new Error() but then thrown at a different location. We might want to capture the variables in this case? But with the current implementation the stack length won't match anyways unless we get lucky -- so we already won't capture variables from this case.

No tests were added, and I imagine maintainers will have a broader depth of knowledge to know if this is a good idea or not - but hoping this gets someone else 90% of the way to fixing this. It functions as expected when used in my application.

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

@Bruno-DaSilva
Copy link
Author

FYI @timfish I see you rerunning CI on this PR - I haven't updated any tests and I see at least some failures in integration tests specific to LocalVariables. I don't have the time to work any more on this, so I was hoping this PR would just be for demo purposes and y'all could take it from here 😄 (and I don't know if you'd prefer a totally different code change)

That cool?

@timfish
Copy link
Collaborator

timfish commented Aug 26, 2024

Yep no problem, thanks for your efforts. I'll get stuck into this tomorrow!

@timfish
Copy link
Collaborator

timfish commented Aug 27, 2024

Closing in favour of #13480 as I couldn't push to this fork!

@timfish timfish closed this Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LocalVariables integration - re-thrown errors can completely mess up frame variables
2 participants