Re: [PATCH] rust: sync: fix incorrect Sync bounds for LockedBy

[Gary Guo]

On Fri, 13 Sep 2024 23:28:37 -0700
Boqun Feng <boqun.feng@xxxxxxxxx> wrote:

> Hmm.. I think it makes more sense to make `access()` requires `where T:
> Sync` instead of the current fix? I.e. I propose we do:
> 
> 	impl<T, U> LockedBy<T, U> {
> 	    pub fn access<'a>(&'a self, owner: &'a U) -> &'a T
> 	    where T: Sync {
> 	    	...
> 	    }
> 	}
> 
> The current fix in this patch disallows the case where a user has a
> `Foo: !Sync`, but want to have multiple `&LockedBy<Foo, X>` in different
> threads (they would use `access_mut()` to gain unique accesses), which
> seems to me is a valid use case.
> 
> The where-clause fix disallows the case where a user has a `Foo: !Sync`,
> a `&LockedBy<Foo, X>` and a `&X`, and is trying to get a `&Foo` with
> `access()`, this doesn't seems to be a common usage, but maybe I'm
> missing something?

+1 on this. Our `LockedBy` type only works with `Lock` -- which
provides mutual exclusion rather than `RwLock`-like semantics, so I
think it should be perfectly valid for people to want to use `LockedBy`
for `Send + !Sync` types and only use `access_mut`. So placing `Sync`
bound on `access` sounds better.

There's even a way to not requiring `Sync` bound at all, which is to
ensure that the owner itself is a `!Sync` type:

	impl<T, U> LockedBy<T, U> {
	    pub fn access<'a, B: Backend>(&'a self, owner: &'a Guard<U, B>) -> &'a T {
	        ...
	    }
	}

Because there's no way for `Guard<U, B>` to be sent across threads, we
can also deduce that all caller of `access` must be from a single
thread and thus the `Sync` bound is unnecessary.

Best,
Gary

> 
> Thoughts?
> 
> Regards,
> Boqun