Enforce panic catching in the Oak LSP - #1406
Conversation
7ef32f4 to
47c2cb1
Compare
b77235a to
5af7b79
Compare
|
|
||
| // We protect from panics to correctly restore `captured_output`'s state. | ||
| // The panic is resumed right after. | ||
| #[expect(clippy::disallowed_methods)] |
There was a problem hiding this comment.
Maybe a comment on this to clarify why we allow a raw catch_unwind here with no boundary? (if I understand the new architecture correctly)
There was a problem hiding this comment.
I think the intent is to let panics thrown from the C debugger to take its course.
But I see below we use resume_unwind(), which doesn't invoke the panic hook, so could leave the process in a bad state (although we're on the main thread so likely not).
I've changed it to properly catch and recover. This is low stake, only used for debugging Ark (and I haven't used these helpers for a long time).
thomasp85
left a comment
thomasp85
left a comment
There was a problem hiding this comment.
Overall LGTM. Do we have any way of ensuring that future tokyo processes are correctly handled so errors in subprocesses can't bring the whole thing down?
In case the future panics
cabb24c to
51c662d
Compare
I've added a lint for |
2 participants
Follow-up to #1405.
This PR enforces good panic catching conventions for Salsa accesses.
All Salsa DB access now go through dedicated accessors that check whether a "panic boundary" (i.e. a catch site) is active on the stack. This ensures that new code/handlers in the Oak LSP are properly guarded against panics.
A clippy rule disallows bare panic catching. Instead the codebase must now use
crate::panic::catch_unwind()which declares a panic boundary.Add boundaries around Tokio tasks and processes.
Positron Release Notes
New Features
Bug Fixes