This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6859

rust-cpython: mark PySharedState as Sync so &'PySharedState can be Send (RFC)
Needs RevisionPublic

Authored by yuja on Sep 16 2019, 7:27 PM.

Details

Reviewers
baymax
Group Reviewers
hg-reviewers
Summary

The goal is to store &'static PySharedState in $leaked struct, which allows
us to move the $leaked struct out of the macro. Currently, it depends on
$inner.py_shared_state(py).

I think PySharedState is Sync because any mutation is synchronized by the
Python GIL, but I'm not pretty sure. I want to know if that's correct before
moving forward.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

yuja created this revision.Sep 16 2019, 7:27 PM
Alphare added a subscriber: Alphare.Sep 17 2019, 5:03 AM

It is indeed Sync because the py_class! macro forces us to have a reference to the GIL at the type level to access the data attributes, but I would also like to be extra sure and have someone else confirm it.

durin42 added a comment.Sep 23 2019, 3:34 PM

I asked some coworkers, and they said:

I do not believe this to be correct. An object is Sync if it is *inherently* thread-safe. This object is not -- the first thing borrow_mut does is a racy read of mutably_borrowed.

so I'm now leaning towards this being a bad idea.

durin42 added a comment.Sep 23 2019, 3:35 PM

Also, since it's possible we could call back into Python code and *that* could release-and-reacquire the GIL, the GIL guarding is probably insufficient to make this a "safe lie".

baymax requested changes to this revision.Jan 24 2020, 12:31 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Jan 24 2020, 12:31 PM

Revision Contents
Changeset List

PathPackages
Mrust/hg-cpython/src/ref_sharing.rs (4 lines)

Diff 16562

rust/hg-cpython/src/ref_sharing.rs

/// Manages the shared state between Python and Rust /// Manages the shared state between Python and Rust
#[derive(Debug, Default)] #[derive(Debug, Default)]
pub struct PySharedState { pub struct PySharedState {
leak_count: Cell<usize>, leak_count: Cell<usize>,
mutably_borrowed: Cell<bool>, mutably_borrowed: Cell<bool>,
} }
// &PySharedState can be Send because any mutation of inner cells is
// synchronized by the GIL.
unsafe impl Sync for PySharedState {}
impl PySharedState { impl PySharedState {
pub fn borrow_mut<'a, T>( pub fn borrow_mut<'a, T>(
&'a self, &'a self,
py: Python<'a>, py: Python<'a>,
pyrefmut: RefMut<'a, T>, pyrefmut: RefMut<'a, T>,
) -> PyResult<PyRefMut<'a, T>> { ) -> PyResult<PyRefMut<'a, T>> {
if self.mutably_borrowed.get() { if self.mutably_borrowed.get() {
return Err(AlreadyBorrowed::new( return Err(AlreadyBorrowed::new(