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

login
register
mail settings
Submitter phabricator
Date Sept. 16, 2019, 11:27 p.m.
Message ID <differential-rev-PHID-DREV-owxo2oc4gwdvbvtydhzv-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/41688/
State New
Headers show

Comments

phabricator - Sept. 16, 2019, 11:27 p.m.
yuja created this revision.
Herald added subscribers: mercurial-devel, kevincox, durin42.
Herald added a reviewer: hg-reviewers.

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6859

AFFECTED FILES
  rust/hg-cpython/src/ref_sharing.rs

CHANGE DETAILS




To: yuja, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel
phabricator - Sept. 17, 2019, 9:04 a.m.
Alphare added a comment.


  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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6859/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6859

To: yuja, #hg-reviewers
Cc: Alphare, durin42, kevincox, mercurial-devel
phabricator - Sept. 23, 2019, 7:34 p.m.
durin42 added a comment.


  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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6859/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6859

To: yuja, #hg-reviewers
Cc: Alphare, durin42, kevincox, mercurial-devel
phabricator - Sept. 23, 2019, 7:35 p.m.
durin42 added a comment.


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

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6859/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6859

To: yuja, #hg-reviewers
Cc: Alphare, durin42, kevincox, mercurial-devel

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 @@ 
     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 {
     pub fn borrow_mut<'a, T>(
         &'a self,