Don't panic if chalk panics #2818
Conversation
|
bors r+ |
2818: Don't panic if chalk panics r=flodiebold a=matklad r? @flodiebold Trying to paper-over panicking chalk. Not sure if this'll make situation better or worse, but I hope it'll be better, as we won't be tearing down type-inference as a whole Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
|
bors r- |
Canceled |
crates/ra_hir_ty/src/traits.rs
Outdated
| // chalk sometimes panics, so let's treat this as a no-solution | ||
| // situation. | ||
| let db = panic::AssertUnwindSafe(db); | ||
| match panic::catch_unwind(|| solver.solve(db.0, &u_canonical)) { |
There was a problem hiding this comment.
hmm, I'm confused, why are we doing the panic handling twice? isn't this just calling the solve method above which already catches panics?
There was a problem hiding this comment.
match self.inner.lock() handles poisoning, panic::catch_unwind handles panics.
Overall, there are four cases here:
- happy path: everyting works, no cancellation happens
- cancellation:
solutionisErr(cancelled), and we need to re-raise this panic (panic::resume_unwind(err)) - an already cancelled request: we try to lock chalk, but the mutex is poisoned. It is poisoned because some other thread got cancelled, and unwound past the mutex guard. Here, we need to initiate unwinding on the current thread
Err(_) => ra_db::Canceled::throw() - chalk panicked. We need to reset the solver
*solver = create_chalk_solver();
At least, I think this is what happens here, I've re-written this like five times, putting catch_unwind in different places. One invariant we have here is that if the mutex is poisoned, it is poisoned because of the cancellation.
There was a problem hiding this comment.
Ok, but there's two catch_unwinds -- the one here and the one above in TraitSolver::solve, which is just what we're calling here, isn't it?
There was a problem hiding this comment.
🤦♂️ that's... actually the remains of one of those five attempts.
Excellent catch!
|
bors r+ |
|
bors seems stuck again. |
|
bors r+ |
2818: Don't panic if chalk panics r=matklad a=matklad r? @flodiebold Trying to paper-over panicking chalk. Not sure if this'll make situation better or worse, but I hope it'll be better, as we won't be tearing down type-inference as a whole Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
Build succeeded
|
3 participants
r? @flodiebold
Trying to paper-over panicking chalk. Not sure if this'll make situation better or worse, but I hope it'll be better, as we won't be tearing down type-inference as a whole