Protect r_task() from panics and detect cancelled handlers#1269
Conversation
b06b7da to
ce5b201
Compare
c692881 to
a05a82d
Compare
ce5b201 to
ccd8780
Compare
DavisVaughan
left a comment
There was a problem hiding this comment.
I feel like I was kind of hoping to see something that prevented us from trying to call salsa queries from an r_task! in the first place. Like it would force us to extract out the relevant info from the salsa queries before moveing the results into the r_task!.
It feels like the Salsa model was:
- main dispatcher gets a request
- secondary thread spawned to handle request
- secondary thread tries to query the salsa db, but something has changed and the query is canceled, causing a panic in secondary thread
and thats totally fine because the secondary thread isnt load bearing
But now we are trying to involve the main R thread from the secondary thread, and the main R thread is able to query salsa, which means it may also get a cancellation panic. And that just feels like we are fighting the idea of salsa (as i understand it) where the salsa queries should be contained within those secondary threads where it is totally fine to panic on cancellation.
a05a82d to
1705d42
Compare
|
I don't think we're fighting the idea of Salsa, more like we're articulating it to how we set up R thread tasks. Also it's all temporary, I went for the easy (and correct) stopgap approach. |
d00f7bc to
5cee100
Compare
1705d42 to
4de75ff
Compare
Branched from #1268
Progress towards #1212
Currently a Salsa cancellation panic can't be triggered in an
r_task()because all calls tor_task()that query Salsa (for completions) happen on the main loop, during which the Salsa DB is pinned (since the main loop is the sole writer). But I thought better to be safe. We now:Catch all panics in
r_task()and reraise them. This is much safer, since it's UB to panic over C frames (top-level-exec and with-calling-handlers frames in particular). If a cancellation is ever raised fromr_task()(e.g. if called from a background thread), it would be safely re-raised the same way, and would be caught byspawn_blocking().Catch Salsa cancellation panics in
respond()and reply to the frontend withContentModified(follows Rust-Analyser).