Skip to content

bug: Avoid panic in Backtrace::catpure if query_stack is already mutably borrowed #835

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

Merged

Conversation

MichaReiser
Copy link
Contributor

This PR fixes a panic in Backtrace::catpure if local_state.query_stack is already mutably borrowed by
returning None.

This was possible because some of our cycle queries used with_query_stack when panicking to include the query stack
and with_query_stack always aquired a mutable borrow. I split with_query_stack into a reference, mutable reference, and try variant
which should avoid this from triggering in most places

@MichaReiser MichaReiser added the bug Something isn't working label Apr 30, 2025
Copy link

netlify bot commented Apr 30, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit c46f2e7
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/6812035f0e6f070008546ed9

Copy link

codspeed-hq bot commented Apr 30, 2025

CodSpeed Performance Report

Merging #835 will not alter performance

Comparing MichaReiser:avoid-panic-in-backtrace-capture (c46f2e7) with master (5b5e982)

Summary

✅ 12 untouched benchmarks

@MichaReiser MichaReiser requested review from davidbarsky and Veykril and removed request for davidbarsky April 30, 2025 11:01
@MichaReiser MichaReiser force-pushed the avoid-panic-in-backtrace-capture branch from eec7f1a to ac99daf Compare April 30, 2025 11:02
@MichaReiser MichaReiser force-pushed the avoid-panic-in-backtrace-capture branch from ac99daf to c46f2e7 Compare April 30, 2025 11:02
@MichaReiser MichaReiser added this pull request to the merge queue Apr 30, 2025
Merged via the queue into salsa-rs:master with commit 42f1583 Apr 30, 2025
11 checks passed
@MichaReiser MichaReiser deleted the avoid-panic-in-backtrace-capture branch April 30, 2025 11:25
@github-actions github-actions bot mentioned this pull request Apr 30, 2025
@Veykril
Copy link
Member

Veykril commented May 1, 2025

That does make this less useful for panic handlers (noticed r-a always aborts now when we panic thanks to the double panic haha)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants