ENQT-GmbH / remoc

Remoc 🦑 — Remote multiplexed objects, channels and RPC for Rust
https://crates.io/crates/remoc
Other
173 stars 16 forks source link

Why is `Client` not `Clone` when method takes &mut self? #11

Closed seeekr closed 1 year ago

seeekr commented 2 years ago

Question above :) Hope this is a good place to ask!

My use case: I have an remoc::rtc server and I want to use the client-side connection from multiple places in the code. As long as the trait has only methods with a &self receiver, the Client is Clone and so I can just .clone() it to share access to it. If I have &mut self methods, then it's not Clone any more... now I'm thinking... does this mean that the Client impl does synchronization on the client side? I would have expect the server to be solely responsible for synchronizing access to its &mut self methods, is that not the case?

EDIT: I also cannot find where in the code it gets decided whether or not the Client will be made Clone, as well as the actual impl of the client. But I'm not used to working with heavy macro usage, so I'm sure I'm just missing the obvious :) EDIT 2: Found where it's determined, don't understand why just yet.

And thanks for making this create, it's quite awesome! Hoping to contribute something of value to it once I've got it in production. e.g. I'm noticing that CallError is not user-extensible, so I can't pass along my own errors from rtc methods, which seems unnecessarily limiting (except that of course each feature needs effort in terms of thinking + impl + maintenance!).

seeekr commented 2 years ago

I'm trying it with always deriving Clone for Client, and at the very least the compiler is not complaining. I'm assuming that it's an old bit of code that hasn't been revisited after some requirement for not deriving Clone has been dropped.

surban commented 2 years ago

Although the server does indeed do synchronization of mutable access, the motivation is to model Rust mutability semantics in the Client object.

Assuming you have a trait with both mutable (&mut self) and immutable methods (&self). You can simultaneously call multiple immutable methods (because multiple immutable references & to the same object can exist), but calls to mutable methods can only be performed in series (because only a single mutable reference &mut can exist). This is one of the core principles of the Rust language.

Now, if Client for a trait with mutable methods would be clonable, this would allow simultaneous calling of mutable methods on the corresponding server object (due to having multiple Client objects). On the server side nothing bad would happen, since the access is serialized. But from the client perspective one object for which an exclusive &mut reference is held, could now "magically" be mutated via another object. Although this would not lead to memory corruption (because the Client objects share no memory on the client), it violates what Rust programmers are used to.

This is why I decided to make traits with mutable self references non-clonable. Technically this restriction could be lifted, but it would introduce the potential for more bugs and unexpected behavior on the client side.

If you need to mutate the server's state with the Client being clonable, I would recommend using a Mutex on the server for the shared state and making all trait methods &self.

seeekr commented 2 years ago

Thanks for the thoughtful response. I don't think I agree with the reasoning here regarding the Client, but I'm not sure that I've thought it through as well as you have. So I'll just leave my current thoughts here and will then return once I feel more confident:

From my perspective the user of this library / of any remote-able trait they're creating with it that the Client represents just a handle to something that happens entirely on a remote server, not unlike an HTTP client, for example, where you would never expect to have any kind of synchronization or otherwise enforcement of ownership semantics on the client side of things. Yes, the RTC feature would potentially make such a thing possible, but it would simply be unintuitive and unexpected, even if it's Rust we're writing here, similarly to how I've experienced this Clone situation.

I would say acting a little like the Client does any kind of enforcement of ownership will even serve to confuse the user. Since the Client API looks like it might be doing that, the user should assume that that is indeed something that is going on behind the scenes. While at the same time it's clear from when you're actually instantiating the Server portion of a remote-able trait, that all of the synchronization and ensuring proper ownership and borrowing rules happens on the server-side.

And also, as a user, I'm scratching my head as to why I should have to figure out a different way to get another handle to a Client than to clone() it, as I would with any other channel (whether one of remoc's, or from many of the other libraries), when clearly the RTC feature and the Client is just based on channels anyway.

That's as much as I can describe so far. I'd like to reiterate that I'm very grateful for the work you've open sourced here, I think there's lots of potential in this library with folks building not-just-local-process applications in Rust!

surban commented 2 years ago

What whould you think about making it possible to add an attribute to the trait that forces the implementation of Clone, even if &mut self methods exist? Although I am not sure this is the right idea because it introduces the inconsistency described in my response above.

surban commented 1 year ago

Please apply the remote attribute to your trait as follows to make the generated client clonable in all cases:

#[rtc::remote(clone)]
pub trait MyTrait {}