Prevent with_critical_section_mutex2 from receiving the same object twice - #6098
Conversation
davidhewitt
left a comment
There was a problem hiding this comment.
Thanks, generally LGTM, some brief thoughts.
| /// Rather than receiving the wrapped data directly, access is gated via the | ||
| /// [`pyo3::sync::critical_section::EnteredCriticalSection`](crate::sync::critical_section::EnteredCriticalSection) | ||
| /// struct. Note that `f` receives an `EnteredCriticalSection<'s, T1>` for the | ||
| /// data protected by `m1` but an `Option<EnteredCriticalSectio<'s, T2>>` for |
There was a problem hiding this comment.
| /// data protected by `m1` but an `Option<EnteredCriticalSectio<'s, T2>>` for | |
| /// data protected by `m1` but an `Option<EnteredCriticalSection<'s, T2>>` for |
| }; | ||
| if core::ptr::eq(m1.mutex.get(), m2.mutex.get()) { | ||
| f(EnteredCriticalSection(&m1.data), None) | ||
| } else { |
There was a problem hiding this comment.
It might be possible to go via the single-critical-section path if the pointers are the same
| } else { | |
| if core::ptr::eq(m1, m2) { | |
| with_critical_section_mutex(|cs| f(cs, None)) | |
| } else { |
... I believe this would avoid the need for the interpreter to also do an equality check on the two mutexes? But the cost of additional function indirection might cancel the benefit out.
| if let Some(ref inner) = b2 { | ||
| assert!(unsafe { *inner.get() }); | ||
| } |
There was a problem hiding this comment.
I wonder if we should do b2.unwrap().get() here given we expect it to be non-none? Same for the other cases, and it looks like we might be missing a test for the new m1 == m2 case?
There was a problem hiding this comment.
and it looks like we might be missing a test for the new m1 == m2 case?
There's already a test for that, test_critical_section2_same_object_no_deadlock. But with this new API deadlocks aren't really a concern anymore so I'll go ahead and change the name.
There was a problem hiding this comment.
I wonder if we should do b2.unwrap().get() here given we expect it to be non-none?
Ah good call, I changed it to match the mutable one above but that can be spelled *b2.as_mut().unwrap().get_mut() which is better anyway. I'll change this one.
Fixes #6051.
Implemented following the suggestions from @Person-93 and @davidhewitt on the issue.