On Sun, Sep 15, 2024 at 04:11:57PM +0200, Alice Ryhl wrote:
On Sun, Sep 15, 2024 at 3:49 PM Gary Guo gary@garyguo.net wrote:
On Fri, 13 Sep 2024 23:28:37 -0700 Boqun Feng boqun.feng@gmail.com 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.
I will add the `where` bound to `access`.
Yeah I considered but it felt a bit icky to put constraints on the functions. But I didn't come up with a real use-case that would be prevented, so I think it's fine.
Even the use-case below where a shared references only gives you the guarantee something is valid you likely have additional locks to protected the data if it's mutable.
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.
Isn't Guard Sync? Either way, it's inconvenient to make Guard part of the interface. That prevents you from using it from within `&self`/`&mut self` methods on the owner.
I think there's also plenty of patterns where just having reference is enoug to guarantee access and exclusive ownership gives exclusive access. E.g. in drm we have some objects that are generally attached to a File, but get independently destroyed. But some of the fields/values are only valid as long as the corresponding File is still around. Lockedby as-is can perfectly encode these kind of rules.
So I don't think tying LockedBy to Guard, or even a specific Lock type is a good idea. -Sima