compile_error test for PyClass: Send

[davidhewitt]

I realised that we have to require Send + Sync for #[pyclass].

Otherwise we violate Rust's thread safety by allowing things like Rc<RefCell<T>> to cross between threads, which isn't good!

[kngwyu]

Because Py<T> can be shared between threads ... ? 🤔
I'm not sure it's problematic.
Maybe we should treat it like Mutex?

[davidhewitt]

See the compile_fail test I added. At the moment it's possible to make this #[pyclass]:

#[pyclass]
struct NotThreadSafe {
    data: Rc<i32>
}

And then yes, PyObject or Py<NotThreadSafe> can be shared between threads.

But accessing the same Rc from two different threads can lead to weird hard to debug erorrs, so this shouldn't ever be possible (I think it's technically undefined behavior?). So we really need to add these bounds, or users can easily shoot themselves in the foot.

[davidhewitt]

Maybe we should treat it like Mutex?

Not sure what you're suggesting with this point?

[kngwyu]

Not sure what you're suggesting with this point?

I had a wrong idea, please don't mind that.

How about adding bounds only for the code related to PyObject/Py?
E.g.,

impl<T: PyClass + Send + Sync> ToPyObject for &PyCell<T> {
    fn to_object(&self, py: Python<'_>) -> PyObject {
        unsafe { PyObject::from_borrowed_ptr(py, self.as_ptr()) }
    }
}

impl<T> Py<T> {
    /// Create a new instance `Py<T>`.
    ///
    /// This method is **soft-duplicated** since PyO3 0.9.0.
    /// Use [`PyCell::new`](../pycell/struct.PyCell.html#method.new) and
    /// `Py::from` instead.
    pub fn new(py: Python, value: impl Into<PyClassInitializer<T>>) -> PyResult<Py<T>>
    where
        T: PyClass + Send + Sync,
        T::BaseLayout: PyBorrowFlagLayout<T::BaseType>,
    {
        let initializer = value.into();
        let obj = unsafe { initializer.create_cell(py)? };
        let ob = unsafe { Py::from_owned_ptr(obj as _) };
        Ok(ob)
    }
}

impl<'a, T> std::convert::From<PyRef<'a, T>> for Py<T>
where
    T: PyClass + Send + Sync,
{
    fn from(pyref: PyRef<'a, T>) -> Self {
        unsafe { Py::from_borrowed_ptr(pyref.as_ptr()) }
    }
}

But if you find this hack not so meaningful, I'm for adding Send + Sync for PyClass.

[davidhewitt]

How about adding bounds only for the code related to PyObject/Py?

Hmm, I'm not sure it's enough. If we give a #[pyclass] object to Python code, then it can always be sent to a new thread without our Rust code ever knowing.

Like the existence of this function:

#[pyclass]
struct NotThreadSafe {
    data: Rc<i32>
}

#[pyfunction]
fn call_from_any_thread(value: &NotThreadSafe) {
    let rc = value.data.clone();
}

Even giving a NotThreadSafe object to Python is problematic in this case, because then Python code can make a new thread and call the function call_from_any_thread without us being able to control it.

So this is why I think the bounds have to go on PyClass.

[davidhewitt]

Also, I'm not sure why the compile_fail test is giving a slightly different output under Windows... I've asked upstream at dtolnay/trybuild#73

Workaround is to just not have a compile_fail test for this, but I think it's nice to have one...

[kngwyu]

Like the existence of this function:

👍
Since this breaking change is relatively easy to fix (by adding unsafe impl ...), I think it's OK to require these bounds.

Also, I'm not sure why the compile_fail test is giving a slightly different output under Windows

Thank you for filing the issue.
I'll also check it and try to find workaround or fix.

[kngwyu]

Rethinking the problem with Python's threading module: though it doesn't work concurrently, isn't it still unsafe to access Rust's !Sync structs from multiple threads?

[davidhewitt]

Rethinking the problem with Python's threading module: though it doesn't work concurrently, isn't it still unsafe to access Rust's !Sync structs from multiple threads?

Yes, which is I think why we need PyClass: Sync as well as Send? Or are you thinking there is further issues we must worry about?

[programmerjake]

Rethinking the problem with Python's threading module: though it doesn't work concurrently, isn't it still unsafe to access Rust's !Sync structs from multiple threads?

Yes, which is I think why we need PyClass: Sync as well as Send? Or are you thinking there is further issues we must worry about?

From what I understand, we only need PyClass: Send since all interior data can only be accessed when the GIL is locked. Sync is for things that can be accessed from multiple threads simultaneously, Send is for things that can be sent between threads or, alternatively, accessed from multiple threads but only by one thread at a time.

Evidence: Sync + Send is implemented for Mutex<T> if T: Send, but T: Sync isn't required:
https://doc.rust-lang.org/std/sync/struct.Mutex.html#impl-Sync

[kngwyu]

@programmerjake
Thank you for the explanation.
I think requiring only Send is reasonable.

In addition, because PyCell is !Sync due to its nature, how about removing these bounds:

unsafe impl Sync for PyObject {}
unsafe impl<T> Sync for Py<T> {}

?

[davidhewitt]

Good catch @programmerjake, I agree you're correct that we need only the Send bound. I'll modify this PR.

In addition, because PyCell is !Sync due to its nature, how about removing these bounds:

I can't see a way a user could write unsafe concurrent access with PyObject or Py<T> shared across threads, because the GIL is needed to do anything with them. So I don't think we need to remove those implementations?

[programmerjake]

Good catch @programmerjake, I agree you're correct that we need only the Send bound. I'll modify this PR.

In addition, because PyCell is !Sync due to its nature, how about removing these bounds:

I can't see a way a user could write unsafe concurrent access with PyObject or Py<T> shared across threads, because the GIL is needed to do anything with them. So I don't think we need to remove those implementations?

Agreed. Was writing a much more confusing reply that was basically saying the same thing when you replied.

[programmerjake]

We will want to make sure that T: Send is included for Py<T> and other similar types, probably the best way is to make Send a supertrait of PyClass since I can't think of any reasons why PyClass + !Send would be useful.

[kngwyu]

I can't see a way a user could write unsafe concurrent access with PyObject or Py shared across threads

I think we can just clone it and move it to another thread, but I'm not sure how it affects the user code now 🤔
So I'll make another PR after playing around some PyO3 crates.
Let's leave it as is for now.

[programmerjake]

As part of the invariant that Py<T>'s data can only be accessed when the GIL is locked, all the member functions should either acquire the GIL or take a GIL token as an argument. Py::get_refcnt and several of the unsafe functions doesn't appear to lock the GIL internally.

Note that it is Undefined Behavior to read the reference count using a plain memory read (what ffi::Py_REFCNT does) while a different thread is concurrently modifying it, making get_refcnt have safety issues.

[davidhewitt]

Changed to just use Send bound. CI is still blocked on trybuild; I've opened a PR which will fix at dtolnay/trybuild#75, waiting for feedback on it.

As part of the invariant that Py's data can only be accessed when the GIL is locked, all the member functions should either acquire the GIL or take a GIL token as an argument. Py::get_refcnt and several of the unsafe functions doesn't appear to lock the GIL internally.

Note that it is Undefined Behavior to read the reference count using a plain memory read (what ffi::Py_REFCNT does) while a different thread is concurrently modifying it, making get_refcnt have safety issues.

Completely agree. I'll open a new issue now to clean up Py<T>. (And I'll probably open a PR to resolve it someday soon too!)

[davidhewitt]

Now that #966 is merged I rebased this to be just the compile_error test which is blocked on trybuild.

[davidhewitt]

Doesn't look like the trybuild PR is moving any time soon...

Also, as we added #[pyclass(Unsendable)] I think this test is probably out of date.

Closing for now and we can perhaps revisit in the future.