Add PyMutex wrappers

[ngoldbaum]

Ref https://github.com/PyO3/pyo3/pull/4504#discussion_r1739869142 and #4265.

• Fixes a spelling error in the PyMutex bindings (oops!)
• Adds pyo3_ffi::PyMutex::new() to allow pyo3 (and users?) to construct PyMutex instances.
• Adds a PyMutex rust wrapper based on the RAII guard pattern.

The test might not be doing anything interesting? I couldn't figure out how to re-acquire the GIL inside of allow_threads and the type errors I was seeing make me think it's not possible.

[mejrs]

It wouldn't be obvious to me, if I knew nothing, how or why this mutex works. The design is very unlike the mutex in std or parking_lot.

We should come up with a couple of good use cases and design the api accordingly.

[mejrs]

(If it is not ok to move it when unlocked, we will need to do more complicated APIs which make use of pinning, I fear...)

This would be easy, we'd just need to change PyMutex::lock to take self: Pin<&mut Self>

[davidhewitt]

(If it is not ok to move it when unlocked, we will need to do more complicated APIs which make use of pinning, I fear...)

This would be easy, we'd just need to change PyMutex::lock to take self: Pin<&mut Self>

Right, yes. I think however that probably we should change .lock() to take &self and wrap the FFI mutex in UnsafeCell?

That way we still cannot move the mutex while locked, and we also make the API more like std or parking_lot. The &self receiver also seems necessary so that multiple threads can actually interact with the mutex.

[mejrs]

Pin would only be necessary if we'd want to enforce that the mutex is never moved once used.

Anyway, this implementation is not an interior mutability primitive. I think most users will expect that.

[ngoldbaum]

It wouldn't be obvious to me, if I knew nothing, how or why this mutex works. The design is very unlike the mutex in std or parking_lot.

We should come up with a couple of good use cases and design the api accordingly.

This mutex has the nice feature that if the thread trying to acquire the mutex blocks and holds the GIL, it will release the GIL. The use case is if you need to lock something while still executing arbitrary python code that might trigger releasing or acquiring the GIL.

It's a good point we should wait to add this until we have more of a need for it. Let's see if it we have a need for it in the internal uses of GILOnceCell that may need to be updated.

[mejrs]

If we want this to be "like std's Mutex, but it releases the gil while it blocks" that's fine with me, but this PR needs a lot of work in that case.

[mejrs]

@colesbury I have a question about the behavior - if we lock it twice on the same thread, say we do

PyMutex mutex = {0};
PyMutex_Lock(&mutex);
PyMutex_Lock(&mutex);
PyMutex_Unlock(&mutex);
PyMutex_Unlock(&mutex);

will this deadlock? I'm guessing yes, because it doesn't say recursive locking is allowed, and it's what I would expect from pthread-like api. I'm asking because if this does not deadlock it is problematic for us (it lets you get multiple MutexGuards and thus aliasing mutable references).

[colesbury]

Yes, it deadlocks

On Wed, Sep 4, 2024 at 8:16 PM Bruno Kolenbrander @.***> wrote:

@colesbury https://github.com/colesbury I have a question about the behavior - if we lock it twice on the same thread, say we do

PyMutex mutex = {0};PyMutex_Lock(&mutex);PyMutex_Lock(&mutex);PyMutex_Unlock(&mutex);PyMutex_Unlock(&mutex);

will this deadlock? I'm guessing yes, because it doesn't say recursive locking is allowed, and it's what I would expect from pthread-like api. I'm asking because if this does not deadlock it is problematic for us (it lets you get multiple MutexGuards and thus aliasing mutable references).

— Reply to this email directly, view it on GitHub https://github.com/PyO3/pyo3/pull/4523#issuecomment-2330360529, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFAD6S4AMRVLMPQ4HBSBF3ZU6PG3AVCNFSM6AAAAABNS3KY3KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZQGM3DANJSHE . You are receiving this because you were mentioned.Message ID: @.***>

[davidhewitt]

We could probably write a test that it deadlocks by spawning a thread and allowing that thread to deadlock, and can assert that thread doesn't run to completion after e.g. 1 second.

[ngoldbaum]

We could probably write a test that it deadlocks by spawning a thread and allowing that thread to deadlock, and can assert that thread doesn't run to completion after e.g. 1 second.

I'm having trouble writing this test because I run into errors like UnsafeCell<pyo3_ffi::PyMutex> cannot be shared between threads safely, so code that calls lock() simultaneously on two threads doesn't compile. It only works if you explicitly move the mutex between threads, as I do in the test.

Am I missing something?

[davidhewitt]

Probably missing unsafe impl<T> Sync for PyMutex<T> {} ?

[mejrs]

Probably missing unsafe impl<T> Sync for PyMutex<T> {} ?

This is incorrect, it must be unsafe impl<T: Send> Sync for PyMutex<T> {}

[ngoldbaum]

Thanks for the hint! With the last push I can write a test that deadlocks:

    #[test]
    fn test_pymutex_deadlocks() {
        static MUTEX: OnceLock<PyMutex<()>> = OnceLock::new();
        static FINISHED: OnceLock<bool> = OnceLock::new();

        MUTEX.get_or_init(|| PyMutex::new(()));

        let _guard = MUTEX.get().unwrap().lock();

        std::thread::spawn(|| {
            MUTEX.get().unwrap().lock();
            FINISHED.get_or_init(|| true);
        })
        .join()
        .unwrap();
    }

but I have no idea how to write a rust test that safely terminates a deadlocked thread after a timeout. Can you point me to an example I can look at somewhere?

[mejrs]

safely terminates a deadlocked thread after a timeout

It doesn't exist because there's no safe way to kill threads in general. This test should be in its own process (is that what integration tests are? I'm unsure).

[ngoldbaum]

Thanks for the suggestions!

Can you elaborate about what API surface from std::sync::Mutex you'd like to see implemented? For example, what about poisoning?

This test should be in its own process (is that what integration tests are? I'm unsure).

Ah good point, if I write a small python wrapper I can do this using pytest.

[codspeed-hq[bot]]

CodSpeed Performance Report

Merging #4523 will not alter performance

_{Comparing ngoldbaum:pymutex-wrappers (bf00ff3) with main (b788b9b)}

Summary

✅ 95 untouched benchmarks

[ngoldbaum]

While working on something else, I realized just now that rather than creating a new mutex.rs, maybe this should live in sync.rs along with the other synchronization primitives.

[mejrs]

Can you elaborate about what API surface from std::sync::Mutex you'd like to see implemented? For example, what about poisoning?

My view is that lock poisoning is useful for propagating panics across threads, so when writing my own code I would like to have it enabled. But I'm not sure what is best in general.

[ngoldbaum]

@davidhewitt and I chatted about this today and he convinced me that because PyO3 automatically converts rust panics into Python exceptions, it's actually more important to deal with poisoning, since the default behavior is not to crash the process.

I'm going to take a look at extending this to use LockResult and PoisonError from the standard library, hopefully it won't be too bad.

[ngoldbaum]

We also decided this doesn't need to block the 0.23 release, since the FFI wrappers also exist.

[ngoldbaum]

I'm thinking of finally finishing this off, because Python 3.14 adds Py_BEGIN_CRITICAL_SECTION_MUTEX and Py_BEGIN_CRITICAL_SECTION2_MUTEX, which accept a PyMutex * argument instead of a PyObject *. If we add safe rust bindings for PyMutex, it becomes much easier in combination with these APIs for people to use the critical section API in situations where you don't have a PyObject but still want to lock the internals of e.g. a rust struct for whatever reason. It also allows using a critical section with a static PyMutex, which is a nice way to add a "local GIL" to a function. I think that's the usecase the TODO in the GILOnceCell internals is referring to:

https://github.com/PyO3/pyo3/blob/aa0fa4c4afb37f17032acf36ede97adfbc532dbd/src/sync.rs#L216-L232

[ngoldbaum]

Taking some notes for myself:

This is all implemented in the standard library using an AtomicBool Flag that hangs off the Mutex object:

https://github.com/rust-lang/rust/blob/246733a3d978de41c5b77b8120ba8f41592df9f1/library/std/src/sync/poison/mutex.rs#L177

And a boolean guard that tracks whether or not a panic happened that hangs off the MutexGuard struct:

https://github.com/rust-lang/rust/blob/246733a3d978de41c5b77b8120ba8f41592df9f1/library/std/src/sync/poison/mutex.rs#L227

Here are the definitions for both structs, which have the meat of the "panic detection" logic:

https://github.com/rust-lang/rust/blob/246733a3d978de41c5b77b8120ba8f41592df9f1/library/std/src/sync/poison.rs#L105-L165

It's a little unclear to me if there are issues with holding a lock and calling into Python, which in turn calls into PyO3, which panics. Doesn't that panic get caught and turned into an exception before the mutex is poisoned? Or do I only need to worry about panics caught before other unwinders are set further down in the call stack?

For the latter, I think it's pretty straightforward to extract the implementation for MutexGuard from the standard library and re-use it for this purpose. The standard library mutex is abstract over the low-level mutex implementation, so it should "just work" to use the standard library implementation but drop all the methods that require try_lock.

We also have PyMutex_IsLocked in the public C API in 3.14, so that should make it easier to write tests as well.

[ngoldbaum]

@davidhewitt @mejrs does that plan sound reasonable to you?

[davidhewitt]

That handling of poisoning seems totally fine to me.

With regard to panics across call stacks, if we PyErr::fetch an exception which looks like it was a panic previously converted to an exception, we'll convert it back to a panic and resume unwinding. So my view is you don't need to worry about this and just follow the standard library design.

So yes, the above sounds good. I guess we need to make a design choice how to name that new critical section API, I guess pyo3::sync::with_mutex_critical_section or pyo3::sync::with_critical_section_mutex seem best?

[ngoldbaum]

I'd use pyo3::sync::with_critical_section_mutex to keep it consistent with the C API naming.

[ngoldbaum]

OK, I think I have poisoning implemented, but I've hit another stumbling block.

I think the with_critical_section_mutex functions really want to accept a PyMutex<T> but then have their closure be an FnOnce(T), but the borrow checker isn't making that easy. See latest commit which fails to compile a test due to a borrow checker error.

Do you see a better design that avoids this issue?

Unfortunately we can't do the same thing we already did with the critical section macros, because we need to access the data wrapped in the PyMutex without locking it.

Should I just do exactly what PyMutex::lock does and use UnsafeCell::raw_get inside an unsafe block?

[ngoldbaum]

Thanks for the code review and the suggestions!

Should we split the PR in two? The PyMutex bits look pretty much ready.

Good point, let's force-push this PR branch to 0c2eb38 and then I'll split off the critical section stuff into its own PR. I'd really like to get that in for PyO3 0.26 but it's not the end of the world if it slips. It just tickles me to add the Rust bindings for C API I proposed and added :)

[ngoldbaum]

I responded to all the open comments.

[ngoldbaum]

I'm leaving all the reviews of the with_critical_section_mutex implementation from the prior iteration unresolved so I can find David's helpful notes later...

[ngoldbaum]

Actually, wait - sorry, going back to the comment above: https://github.com/PyO3/pyo3/pull/4523#discussion_r1743922232

The Rust standard library mutex is !Send, so we definitely do need PhantomPinned:

https://github.com/rust-lang/rust/blob/246733a3d978de41c5b77b8120ba8f41592df9f1/library/std/src/sync/poison/mutex.rs#L232-L237

Does this also mean that the rust wrapper needs a PhantomPinned as well?