RFC: Add GILProtected synchronization primitive and use it for LazyTypeObjectInner.

[adamreichold]

I would also like to use that type in rust-numpy and it seems we can avoid both a manual unsafe impl and a full blown mutex if we apply it to LazyTypeObjectInner.

One downside might be that it ties us closer to the GIL when we want to enable nogil experimentation, but on the other hand, it may also help by reifying the GIL usage. (This is currently limited to comments in unsafe code in rust-numpy for example.)

[mejrs]

I think this PR title and the documentation are deceiving. This isn't a mutex - it's just a wrapper for making a !Sync type implement Sync.

[adamreichold]

I think this PR title and the documentation are deceiving. This isn't a mutex - it's just a wrapper for making a !Sync type implement Sync.

But isn't this what a mutex does, i.e. https://doc.rust-lang.org/stable/std/sync/struct.Mutex.html#impl-Sync-for-Mutex%3CT%3E ?

The mechanism is different of course, but as with std's Mutex, you can only access the inner value while holding the lock. Acquiring the lock and accessing the value is just more disconnected than with the std's Mutex but still reified in the type system via the Python token.

[adamreichold]

One thing that is still unsound though, is that I cannot return a bare reference, i.e. with_gil must return a !Send reference wrapper so that I cannot e.g. pass that reference into an allow_threads closure.

[adamreichold]

One thing that is still unsound though, is that I cannot return a bare reference, i.e. with_gil must return a !Send reference wrapper so that I cannot e.g. pass that reference into an allow_threads closure.

Actually, I might have been confused here: If T: !Sync, i.e. &T: !Send, the returned bare reference is already pinned to the current thread. In the concrete example, &RefCell cannot be send to another thread in the first place. (Of course, if T: Sync then it works, but then the GIL is unnecessary in the first place.)

[adamreichold]

Meaning that I do not need a reference wrapper after all.

Maybe another way to think about this is that GILProtected is basically the same thing we do at each #[pyfunction] entry when we provide shared references to values stored on the Python heap by ensuring via the GIL that only one threads accesses them at any time, just as a free-standing type and without handling how to get hold of the Python token in the first place.

[mejrs]

This is a synchronization primitive, but it's not a mutex. And not all synchronization primitives implement Sync like this. RwLock doesn't, for example.

Also, mutex means "mutually exclusive". This does not give you something exclusive, because you can call it multiple times as long as you are holding the gil. Calling it a mutex is just going to confuse people when they try swapping out their mutex but can't get a &mut T out of it. Something like GilWrapper would be a better name.

[adamreichold]

Also, mutex means "mutually exclusive". Also, mutex means "mutually exclusive". This does not give you something exclusive, because you can call it multiple times as long as you are holding the gil.

Well, it is mutually exclusive insofar as only one thread can hold the GIL, so one might call it "recursive mutex" in that schema.

Calling it a mutex is just going to confuse people when they try swapping out their mutex but can't get a &mut T out of it. Something like GilWrapper would be a better name.

That said, I am open to give this a different name and/or removing the reference to Mutex from the comments. GilWrapper does not seem helpful IMHO though as it does not indicate what the wrapping achieves.

What about changing the doc comment to

/// This is a synchronization primitive based on Python's global interpreter lock (GIL).
/// It ensures that only one thread at a time can access the inner value via shared references.
/// It can be combined with interior mutability to obtain mutable references.

?

[davidhewitt]

This seems like a very reasonable idea to me. I like the way that it gives us an API which we can encourage downstream code to unify around!

cc @colesbury - might be interesting to see this RE nogil context

[davidhewitt]

Should this perhaps live in a pyo3::sync module along with GILOnceCell?

[adamreichold]

Should this perhaps live in a pyo3::sync module along with GILOnceCell?

Yes, will move it there. (I managed to edit the original comment instead of replying to it. Sorry for that.)

[mejrs]

Overall I like this, I have some nits though.

[davidhewitt]

I'm thinking this would be nice to include in 0.18.2?

[adamreichold]

I'm thinking this would be nice to include in 0.18.2?

Isn't replacing an item with a re-export technically a breaking change? (Note to self, we should probably setup cargo-semver-checks...)

[davidhewitt]

I'm thinking this would be nice to include in 0.18.2?

Isn't replacing an item with a re-export technically a breaking change? (Note to self, we should probably setup cargo-semver-checks...)

Is there an observable compile issue that can be introduced by moving the item and re-exporting from the old location? I would hope that this would be unnoticeable from a user protective 🙂

[adamreichold]

I'm thinking this would be nice to include in 0.18.2?

Isn't replacing an item with a re-export technically a breaking change? (Note to self, we should probably setup cargo-semver-checks...)

Is there an observable compile issue that can be introduced by moving the item and re-exporting from the old location? I would hope that this would be unnoticeable from a user protective slightly_smiling_face

I was thinking of https://predr.ag/blog/moving-and-reexporting-rust-type-can-be-major-breaking-change/ but that seems to apply only to using pub type instead of pub use for re-exporting. So it should not be a problem in this case here.

[davidhewitt]

👍 let's ship it!

bors r+

[bors]

Build succeeded:

• conclusion