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

Make Capsule preserve wrapped exception backtraces #3421

Merged
merged 8 commits into from
Jan 6, 2025

Conversation

TheNumbat
Copy link
Contributor

Reraising a newly constructed exception creates a fresh backtrace.
We want to preserve the existing backtrace when encapsulating or protecting an exception raised from a capsule function.

@TheNumbat TheNumbat changed the title Capsule raises wrapped exceptions with existing backtrace Make Capsule preserve wrapped exception backtraces Jan 2, 2025
Copy link
Contributor

@tdelvecchio-jsc tdelvecchio-jsc left a comment

Choose a reason for hiding this comment

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

Looks good.

Base automatically changed from capsule-with-password to main January 3, 2025 01:10
@lpw25
Copy link
Collaborator

lpw25 commented Jan 3, 2025

We may need to be careful here. You might be getting quadratic performance by repeatedly copying the backtraces in and out of the backtrace buffer. It would probably be better to add a new raise primitive that does the right thing.

@TheNumbat
Copy link
Contributor Author

This sort of quadratic behavior is already present in Fun.protect and Base.Exn.protect, but I could see it being more of a problem when used in nested capsules/parallel tasks. In my use case the backtraces don't grow at each reraise, so I think it's okay to merge this for now, but I'll add a CR about adding a raise-with-current-backtrace primitive.

@TheNumbat TheNumbat merged commit 2e49469 into main Jan 6, 2025
24 checks passed
@TheNumbat TheNumbat deleted the capsule-backtraces branch January 6, 2025 18:05
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.

3 participants