Patchwork [03,of,10] rust-cpython: mark PySharedState as Sync so &'PySharedState can be Send

login
register
mail settings
Submitter Yuya Nishihara
Date Sept. 22, 2019, 6:41 a.m.
Message ID <fcaf01804ac8198d52d1.1569134500@mimosa>
Download mbox | patch
Permalink /patch/41722/
State Accepted
Headers show

Comments

Yuya Nishihara - Sept. 22, 2019, 6:41 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1568674765 -32400
#      Tue Sep 17 07:59:25 2019 +0900
# Node ID fcaf01804ac8198d52d141d642d5ef7c935360cc
# Parent  34a355c69f643f15ff46f4524c75137c08a8de3b
rust-cpython: mark PySharedState as Sync so &'PySharedState can be Send

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.$data_member(py), which can't be generalized.

PySharedState is Sync because any mutation or read operation is synchronized
by the Python GIL, py: Python<'a>, which should guarantee that &'PySharedState
can be sent to another thread.
Augie Fackler - Sept. 23, 2019, 7:37 p.m.
> On Sep 22, 2019, at 02:41, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1568674765 -32400
> #      Tue Sep 17 07:59:25 2019 +0900
> # Node ID fcaf01804ac8198d52d141d642d5ef7c935360cc
> # Parent  34a355c69f643f15ff46f4524c75137c08a8de3b
> rust-cpython: mark PySharedState as Sync so &'PySharedState can be Send

I asked some colleagues in the context  of D6859, airlifting my comments from phabricator:

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.

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".

(I didn't look through the rest of this series in detail - this alone was enough for me to be sad and shuffle back to other things.)

> 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.$data_member(py), which can't be generalized.
> 
> PySharedState is Sync because any mutation or read operation is synchronized
> by the Python GIL, py: Python<'a>, which should guarantee that &'PySharedState
> can be sent to another thread.
> 
> diff --git a/rust/hg-cpython/src/ref_sharing.rs b/rust/hg-cpython/src/ref_sharing.rs
> --- a/rust/hg-cpython/src/ref_sharing.rs
> +++ b/rust/hg-cpython/src/ref_sharing.rs
> @@ -18,6 +18,10 @@ pub struct PySharedState {
>     mutably_borrowed: Cell<bool>,
> }
> 
> +// &PySharedState can be Send because any access to inner cells is
> +// synchronized by the GIL.
> +unsafe impl Sync for PySharedState {}
> +
> impl PySharedState {
>     pub fn borrow_mut<'a, T>(
>         &'a self,
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Sept. 23, 2019, 10:50 p.m.
On Mon, 23 Sep 2019 15:37:07 -0400, Augie Fackler wrote:
> > On Sep 22, 2019, at 02:41, Yuya Nishihara <yuya@tcha.org> wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1568674765 -32400
> > #      Tue Sep 17 07:59:25 2019 +0900
> > # Node ID fcaf01804ac8198d52d141d642d5ef7c935360cc
> > # Parent  34a355c69f643f15ff46f4524c75137c08a8de3b
> > rust-cpython: mark PySharedState as Sync so &'PySharedState can be Send
> 
> I asked some colleagues in the context  of D6859, airlifting my comments from phabricator:
> 
> 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.
> 
> 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".

My understanding is that Python<'a> is basically a mutex, and no other
Python thread is running while it is taken.

I can add some Mutex to make PySharedState inherently be Sync, but do we
need such thing?
Augie Fackler - Sept. 24, 2019, 8:36 p.m.
> On Sep 23, 2019, at 18:50, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Mon, 23 Sep 2019 15:37:07 -0400, Augie Fackler wrote:
>>> On Sep 22, 2019, at 02:41, Yuya Nishihara <yuya@tcha.org> wrote:
>>> # HG changeset patch
>>> # User Yuya Nishihara <yuya@tcha.org>
>>> # Date 1568674765 -32400
>>> #      Tue Sep 17 07:59:25 2019 +0900
>>> # Node ID fcaf01804ac8198d52d141d642d5ef7c935360cc
>>> # Parent  34a355c69f643f15ff46f4524c75137c08a8de3b
>>> rust-cpython: mark PySharedState as Sync so &'PySharedState can be Send
>> 
>> I asked some colleagues in the context  of D6859, airlifting my comments from phabricator:
>> 
>> 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.
>> 
>> 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".
> 
> My understanding is that Python<'a> is basically a mutex, and no other
> Python thread is running while it is taken.

That's right, with the caveat that if we have a situation where:

Python -> Rust (has GIL) -> Python -> Rust (drops GIL)

we're now potentially in a weird place. I'm not sure if I should worry about this. Supposedly we should spend some time browsing soundness bugs in PyO3 to get a sense of things?

> I can add some Mutex to make PySharedState inherently be Sync, but do we
> need such thing?

I'm honestly not sure anymore. :(
Yuya Nishihara - Sept. 24, 2019, 11:28 p.m.
On Tue, 24 Sep 2019 16:36:17 -0400, Augie Fackler wrote:
> > On Sep 23, 2019, at 18:50, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Mon, 23 Sep 2019 15:37:07 -0400, Augie Fackler wrote:
> >>> On Sep 22, 2019, at 02:41, Yuya Nishihara <yuya@tcha.org> wrote:
> >>> # HG changeset patch
> >>> # User Yuya Nishihara <yuya@tcha.org>
> >>> # Date 1568674765 -32400
> >>> #      Tue Sep 17 07:59:25 2019 +0900
> >>> # Node ID fcaf01804ac8198d52d141d642d5ef7c935360cc
> >>> # Parent  34a355c69f643f15ff46f4524c75137c08a8de3b
> >>> rust-cpython: mark PySharedState as Sync so &'PySharedState can be Send
> >> 
> >> I asked some colleagues in the context  of D6859, airlifting my comments from phabricator:
> >> 
> >> 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.
> >> 
> >> 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".
> > 
> > My understanding is that Python<'a> is basically a mutex, and no other
> > Python thread is running while it is taken.
> 
> That's right, with the caveat that if we have a situation where:
> 
> Python -> Rust (has GIL) -> Python -> Rust (drops GIL)

I expect rust-cpython would prohibit that within the safe API, but I'm not
an expert. Raphaël, do you have any thought?
Yuya Nishihara - Oct. 5, 2019, 3:03 p.m.
On Sun, 22 Sep 2019 15:41:40 +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1568674765 -32400
> #      Tue Sep 17 07:59:25 2019 +0900
> # Node ID fcaf01804ac8198d52d141d642d5ef7c935360cc
> # Parent  34a355c69f643f15ff46f4524c75137c08a8de3b
> rust-cpython: mark PySharedState as Sync so &'PySharedState can be Send
> 
> 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.$data_member(py), which can't be generalized.
> 
> PySharedState is Sync because any mutation or read operation is synchronized
> by the Python GIL, py: Python<'a>, which should guarantee that &'PySharedState
> can be sent to another thread.
> 
> diff --git a/rust/hg-cpython/src/ref_sharing.rs b/rust/hg-cpython/src/ref_sharing.rs
> --- a/rust/hg-cpython/src/ref_sharing.rs
> +++ b/rust/hg-cpython/src/ref_sharing.rs
> @@ -18,6 +18,10 @@ pub struct PySharedState {
>      mutably_borrowed: Cell<bool>,
>  }
>  
> +// &PySharedState can be Send because any access to inner cells is
> +// synchronized by the GIL.
> +unsafe impl Sync for PySharedState {}

Perhaps, this unsafe impl can be removed later.

If we take the generation-poisoning idea, PySharedState will be just
{ generation: AtomicUsize }, which is inherently Sync.
Raphaël Gomès - Oct. 5, 2019, 3:15 p.m.
Poisoning is the way to go in the long run. I haven't really thought 
about how much of a pain this could be.

On 10/5/19 5:03 PM, Yuya Nishihara wrote:
> On Sun, 22 Sep 2019 15:41:40 +0900, Yuya Nishihara wrote:
>> # HG changeset patch
>> # User Yuya Nishihara <yuya@tcha.org>
>> # Date 1568674765 -32400
>> #      Tue Sep 17 07:59:25 2019 +0900
>> # Node ID fcaf01804ac8198d52d141d642d5ef7c935360cc
>> # Parent  34a355c69f643f15ff46f4524c75137c08a8de3b
>> rust-cpython: mark PySharedState as Sync so &'PySharedState can be Send
>>
>> 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.$data_member(py), which can't be generalized.
>>
>> PySharedState is Sync because any mutation or read operation is synchronized
>> by the Python GIL, py: Python<'a>, which should guarantee that &'PySharedState
>> can be sent to another thread.
>>
>> diff --git a/rust/hg-cpython/src/ref_sharing.rs b/rust/hg-cpython/src/ref_sharing.rs
>> --- a/rust/hg-cpython/src/ref_sharing.rs
>> +++ b/rust/hg-cpython/src/ref_sharing.rs
>> @@ -18,6 +18,10 @@ pub struct PySharedState {
>>       mutably_borrowed: Cell<bool>,
>>   }
>>   
>> +// &PySharedState can be Send because any access to inner cells is
>> +// synchronized by the GIL.
>> +unsafe impl Sync for PySharedState {}
> Perhaps, this unsafe impl can be removed later.
>
> If we take the generation-poisoning idea, PySharedState will be just
> { generation: AtomicUsize }, which is inherently Sync.

Patch

diff --git a/rust/hg-cpython/src/ref_sharing.rs b/rust/hg-cpython/src/ref_sharing.rs
--- a/rust/hg-cpython/src/ref_sharing.rs
+++ b/rust/hg-cpython/src/ref_sharing.rs
@@ -18,6 +18,10 @@  pub struct PySharedState {
     mutably_borrowed: Cell<bool>,
 }
 
+// &PySharedState can be Send because any access to inner cells is
+// synchronized by the GIL.
+unsafe impl Sync for PySharedState {}
+
 impl PySharedState {
     pub fn borrow_mut<'a, T>(
         &'a self,