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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

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

indygreg (Gregory Szorc)
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

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,



To: yuja, #hg-reviewers
Cc: durin42, kevincox, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

indygreg (Gregory Szorc)
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
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

indygreg (Gregory Szorc)
In reply to this post by indygreg (Gregory Szorc)
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
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

indygreg (Gregory Szorc)
In reply to this post by indygreg (Gregory Szorc)
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
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel