| 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
> 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
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?
> 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. :(
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?
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.
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,