make PyErrState thread-safe
#4671
Conversation
|
I noticed clippy was failing so I just pushed a fix. I'll try to get the CI green on this if there are any more issues. |
src/err/err_state.rs
Outdated
| match self_state { | ||
| Some(PyErrStateInner::Normalized(n)) => n, | ||
| _ => unreachable!(), | ||
| let normalized_state = PyErrStateInner::Normalized(state.normalize(py)); |
There was a problem hiding this comment.
I think the only spot where there might be a deadlock is here, if normalize somehow leads to arbitrary Python code execution.
Is that possible? If not I think it deserves a comment explaining why.
There was a problem hiding this comment.
If it can deadlock, I'm not sure what we can do, since at this point we haven't actually constructed any Python objects yet and we only have a handle to an FnOnce that knows how to construct them.
There was a problem hiding this comment.
Great observation; I've added a wrapping call to py.allow_threads before potentially blocking on the Once, which I think avoids the deadlock (I pushed a test which did deadlock before that change).
|
The algorithm makes sense to me, I agree that this ensures that normalizing an error state can't be done simultaneously in two threads. |
CodSpeed Performance ReportMerging #4671 will degrade performances by 25.38%Comparing Summary
Benchmarks breakdown
|
|
Huh, I can reproduce the test failure happening on CI. It's flakey, but you can trigger it with |
| .expect("Cannot normalize a PyErr while already normalizing it.") | ||
| }; | ||
| // avoid deadlock of `.call_once` with the GIL | ||
| py.allow_threads(|| { |
There was a problem hiding this comment.
I guess somehow dropping the GIL somehow allows a race condition to happen where multiple threads try to simultaneously create a module...
There was a problem hiding this comment.
Yeah, I think it's a combination with GILOnceCell in the test_declarative_module; we allow racing in GILOnceCell under the condition where switching the GIL, so this module does actually attempt to get created multiple times. I think it's a bug in using GILOnceCell for that test, but this also just makes me dislike this lazy stuff even more...
There was a problem hiding this comment.
I guess this is just a fundamental issue with GILOnceCell being racey if the code it wraps ever drops the GIL.
EDIT: jinx!
There was a problem hiding this comment.
I've opened #4676, if I apply that patch on this branch, the problem goes away.
This PR resolves the thread-safety challenges of #4584 for us to be able to at least ship 0.23.
I don't love the complexity that this lazy state creates inside error-handling pathways, so I think in the future I will work to proceed with #4669 and further steps to remove the lazy state. But 0.23 is already breaking enough, users don't need more changes and this should be an in-place drop-in.