Add PyMutex wrappers #4523

ngoldbaum · 2024-09-03T20:14:30Z

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.

davidhewitt

Thanks very much for implementing this! I have a bunch of initial thoughts, mostly we should understand what the invariant around moving the FFI mutex is before we worry too much about the fine details. (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...)

davidhewitt · 2024-09-04T12:17:32Z

src/types/mutex.rs

+    }
+
+    /// Create a new mutex in an unlocked state ready for use.
+    pub fn new(value: T) -> Self {


This can be const

Suggested change

pub fn new(value: T) -> Self {

pub const fn new(value: T) -> Self {

davidhewitt · 2024-09-04T12:21:35Z

src/types/mutex.rs

+/// Wrapper for [`PyMutex`](https://docs.python.org/3/c-api/init.html#c.PyMutex) exposing an RAII interface.
+#[derive(Debug)]
+pub struct PyMutex<T> {
+    _mutex: crate::ffi::PyMutex,


This design would permit moving the mutex while it's unlocked. As far as the C API docs state, they just say the mutex must never be moved. @colesbury is it permissible to move the mutex while it's unlocked?

Yeah, it's the same restrictions as pthread_mutex_t. I think that's fine if Rust allows it for std::sync::mutex.

I read the C api documentation as "once you create/use this, you can never move it again", which is why I pushed for the PhantomPinned. If it really means "Instances of PyMutex should not be copied or moved while it is locked.", we can remove that.

Also, while I appreciate your clarification here, would you mind adding it to https://docs.python.org/3.14/c-api/init.html#c.PyMutex ? I would feel a lot better if I can point to official documentation rather than a comment on a PR.

davidhewitt · 2024-09-04T12:25:33Z

src/types/mutex.rs

+use std::ops::Deref;
+
+/// Wrapper for [`PyMutex`](https://docs.python.org/3/c-api/init.html#c.PyMutex) exposing an RAII interface.
+#[derive(Debug)]


I think a derived Debug implementation here is unsound as it could allow reading while another thread is writing. We should check what the debug implementation for std::sync::Mutex prints.

Std's Mutex' Debug impl uses try_lock, which PyMutex doesn't have. We can have a Debug impl, it just can't print any of its contents.

davidhewitt · 2024-09-04T12:27:20Z

src/types/mutex.rs

@@ -0,0 +1,86 @@
+use std::ops::Deref;
+
+/// Wrapper for [`PyMutex`](https://docs.python.org/3/c-api/init.html#c.PyMutex) exposing an RAII interface.


Here we should probably advise that the advantage of this is that it automatically detaches from python thread states if necessary.

src/types/mutex.rs

mejrs · 2024-09-04T15:25:20Z

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 · 2024-09-04T15:28:57Z

(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 · 2024-09-04T15:51:32Z

(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 · 2024-09-04T15:57:31Z

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 · 2024-09-04T20:25:02Z

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 · 2024-09-04T23:45:09Z

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 · 2024-09-05T00:16:22Z

@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 · 2024-09-05T00:59:42Z

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 <#4523 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFAD6S4AMRVLMPQ4HBSBF3ZU6PG3AVCNFSM6AAAAABNS3KY3KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZQGM3DANJSHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

alex · 2024-09-05T04:02:55Z

src/types/mutex.rs

+#[derive(Debug)]
+pub struct PyMutex<T> {
+    _mutex: crate::ffi::PyMutex,
+    data: T,


This should probably be UnsafeCell<T>

davidhewitt · 2024-09-05T09:58:50Z

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.

davidhewitt · 2024-09-13T15:10:29Z

src/types/mutex.rs

+pub struct PyMutex<T> {
+    _mutex: crate::ffi::PyMutex,
+    data: T,
+}


Suggested change

pub struct PyMutex<T> {

_mutex: crate::ffi::PyMutex,

data: T,

}

pub struct PyMutex<T> {

_mutex: UnsafeCell<crate::ffi::PyMutex>,

data: UnsafeCell<T>,

}

both cells are needed; one for lock / unlock, and one for data to be readable from interior mutability.

davidhewitt · 2024-09-13T15:18:46Z

src/types/mutex.rs

+    _mutex: &'a mut crate::ffi::PyMutex,
+    data: &'a T,


Suggested change

_mutex: &'a mut crate::ffi::PyMutex,

data: &'a T,

mutex: &'a PyMutex<T>,

... and then can read from inside the mutex's UnsafeCell for the Deref and DerefMut implementations.

ngoldbaum · 2024-09-17T21:23:25Z

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 · 2024-09-17T21:27:38Z

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

mejrs · 2024-09-17T22:08:07Z

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

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

ngoldbaum · 2024-09-17T22:28:33Z

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

We should get consensus on what we want this api to look like; do we want it to mirror std's mutex? (the reviews so far assume as much).

The Send and Sync (un)implementations are quite tricky to get right. Even std got this wrong initially. I think the implementations of this Mutex and MutexGuard should be the same as std's.

mejrs · 2024-09-17T22:14:25Z

pyo3-ffi/src/cpython/lock.rs

@@ -8,6 +8,15 @@ pub struct PyMutex {
    pub(crate) _pin: PhantomPinned,


This not part of the PR but since colesbury's clarification I believe this is unnecessary, so it can be removed.

mejrs · 2024-09-17T22:15:48Z

src/types/mutex.rs

+use std::ops::Deref;
+
+/// Wrapper for [`PyMutex`](https://docs.python.org/3/c-api/init.html#c.PyMutex) exposing an RAII interface.
+#[derive(Debug)]


Std's Mutex' Debug impl uses try_lock, which PyMutex doesn't have. We can have a Debug impl, it just can't print any of its contents.

mejrs · 2024-09-17T22:16:01Z

src/types/mutex.rs

+/// RAII guard to handle releasing a PyMutex lock.
+#[derive(Debug)]
+pub struct PyMutexGuard<'a, T> {
+    mutex: &'a mut PyMutex<T>,


Suggested change

mutex: &'a mut PyMutex<T>,

mutex: &'a PyMutex<T>,

mejrs · 2024-09-17T22:17:36Z

src/types/mutex.rs

+/// Wrapper for [`PyMutex`](https://docs.python.org/3/c-api/init.html#c.PyMutex), exposing an RAII guard interface similar to `std::sync::Mutex`.
+#[derive(Debug)]
+pub struct PyMutex<T> {
+    _mutex: UnsafeCell<crate::ffi::PyMutex>,


Suggested change

_mutex: UnsafeCell<crate::ffi::PyMutex>,

inner: UnsafeCell<crate::ffi::PyMutex>,

To avoid a bunch of self.mutex_mutex or self.mutex.mutex (if the underscore were to be removed)

mejrs · 2024-09-17T22:20:31Z

src/types/mutex.rs

+use std::cell::UnsafeCell;
+use std::ops::{Deref, DerefMut};
+
+/// Wrapper for [`PyMutex`](https://docs.python.org/3/c-api/init.html#c.PyMutex), exposing an RAII guard interface similar to `std::sync::Mutex`.


This should go into depth about how it's different from std's Mutex as well.

mejrs · 2024-09-17T22:25:23Z

src/types/mutex.rs

+
+/// Wrapper for [`PyMutex`](https://docs.python.org/3/c-api/init.html#c.PyMutex), exposing an RAII guard interface similar to `std::sync::Mutex`.
+#[derive(Debug)]
+pub struct PyMutex<T> {


Suggested change

pub struct PyMutex<T> {

pub struct PyMutex<T: ?Sized> {

We should be able to support unsized types, I think.

mejrs · 2024-09-17T22:41:57Z

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 · 2024-09-17T23:46:12Z

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 · 2024-09-19T20:53:13Z

CodSpeed Performance Report

Merging #4523 will not alter performance

_{Comparing ngoldbaum:pymutex-wrappers (22ee351) with main (3b39a83)}

Summary

✅ 81 untouched benchmarks

ngoldbaum · 2024-09-26T18:39:00Z

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.

ngoldbaum · 2024-09-30T22:38:45Z

src/types/mutex.rs

+        let first_thread_locked_once = AtomicBool::new(false);
+        let second_thread_locked_once = AtomicBool::new(false);
+        let finished = AtomicBool::new(false);
+        let (sender, receiver) = sync_channel::<bool>(0);


This should be a std::sync::Barrier

mejrs · 2024-10-01T11:40:10Z

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.

adamcavendish · 2024-10-05T12:49:01Z

src/types/mutex.rs

+
+/// Wrapper for [`PyMutex`](https://docs.python.org/3/c-api/init.html#c.PyMutex), exposing an RAII guard interface.
+///
+/// Comapred with `std::sync::Mutex` or `parking_lot::Mutex`, this is a very


Comapred -> Compared

davidhewitt reviewed Sep 4, 2024

View reviewed changes

alex reviewed Sep 5, 2024

View reviewed changes

ngoldbaum mentioned this pull request Sep 5, 2024

Fix typo and missed configs in FFI bindings #4532

Merged

davidhewitt reviewed Sep 13, 2024

View reviewed changes

ngoldbaum added 5 commits September 17, 2024 14:13

define public pyo3_ffi::PyMutex::new()

4865289

add pyo3::types::mutex for PyMutex wrappers

874d608

add changelog entry

b1afb8b

update rust API for PyMutex bindings following code review

b3d193b

make the test do something more interesting

1229634

ngoldbaum force-pushed the pymutex-wrappers branch from 41e87b3 to 1229634 Compare September 17, 2024 21:23

add Sync impl and make lock accept an immutable reference

307e8cf

mejrs requested changes Sep 17, 2024

View reviewed changes

ngoldbaum added 3 commits September 19, 2024 11:40

respond to review comments

f0ead33

add test for PyMutex blocking behavior

4a0bf3b

expand docstrings

17263cc

ngoldbaum added 4 commits September 19, 2024 13:43

fix clippy under abi3

37ffa3a

simplify test

10f5faf

use AtomicBool instead of OnceLock<bool>

7df69a0

use std::hint::spin_loop() per clippy lint

e35ea76

ngoldbaum added 2 commits September 19, 2024 15:00

make PyMutex::new() const

604b9bb

fix docstring formatting

22ee351

ngoldbaum marked this pull request as ready for review September 20, 2024 18:26

ngoldbaum mentioned this pull request Sep 20, 2024

0.23 Release #4554

Open

3 tasks

ngoldbaum commented Sep 30, 2024

View reviewed changes

adamcavendish reviewed Oct 5, 2024

View reviewed changes

	pub fn new(value: T) -> Self {
	pub const fn new(value: T) -> Self {

		@@ -0,0 +1,86 @@
		use std::ops::Deref;

		/// Wrapper for [`PyMutex`](https://docs.python.org/3/c-api/init.html#c.PyMutex) exposing an RAII interface.

	_mutex: &'a mut crate::ffi::PyMutex,
	data: &'a T,
	mutex: &'a PyMutex<T>,

		@@ -8,6 +8,15 @@ pub struct PyMutex {
		pub(crate) _pin: PhantomPinned,

	_mutex: UnsafeCell<crate::ffi::PyMutex>,
	inner: UnsafeCell<crate::ffi::PyMutex>,

Add PyMutex wrappers #4523

Are you sure you want to change the base?

Add PyMutex wrappers #4523

Conversation

ngoldbaum commented Sep 3, 2024

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mejrs commented Sep 4, 2024

mejrs commented Sep 4, 2024

davidhewitt commented Sep 4, 2024

mejrs commented Sep 4, 2024

ngoldbaum commented Sep 4, 2024

mejrs commented Sep 4, 2024

mejrs commented Sep 5, 2024

colesbury commented Sep 5, 2024 via email

Choose a reason for hiding this comment

davidhewitt commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngoldbaum commented Sep 17, 2024 • edited Loading

davidhewitt commented Sep 17, 2024

mejrs commented Sep 17, 2024

ngoldbaum commented Sep 17, 2024

mejrs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mejrs commented Sep 17, 2024 • edited Loading

ngoldbaum commented Sep 17, 2024

codspeed-hq bot commented Sep 19, 2024 • edited Loading

Merging #4523 will not alter performance

Summary

ngoldbaum commented Sep 26, 2024

Choose a reason for hiding this comment

mejrs commented Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

ngoldbaum commented Sep 17, 2024 •

edited

Loading

mejrs commented Sep 17, 2024 •

edited

Loading

codspeed-hq bot commented Sep 19, 2024 •

edited

Loading

mejrs commented Oct 1, 2024 •

edited

Loading