Implementing Send markers for RpcSystem

[b00f]

RpcSystem can be spawned locally and on the same thread that called it. To use spawn and running it on another thread, the Sync maker should be implemented.

[teythoon]

I'm also curious where this limitation comes from, and love to see it adressed because as I understand it, this makes using the rpc system somewhat unergonomic.

[Kilobyte22]

This is actually because the capnp data structures themselves are !Send and !Sync. I have traced it down to them including raw pointers, which are always !Send and !Sync by default. I am not quite sure and this would need further investigations, but to me it looks like making those data types Send (unsafe impl Send for X for those PointerReaders and the like) would actually be safe. Sync very likely not though, as there is probably some interior mutability going on. reborrow() does look a bit fishy to me, but i think it might be fine.

(Written after some grief of having to make an entire application run effectively single threaded)

[b00f]

I remembered I got confused with reborrow method. This is a nice project, but I believe it needs some kind of refactoring.

[b00f]

Do you think replacing Rc<RefCell<...>> by Arc<Mutex<...>>in ClientVariant will address this issue?

@Kilobyte22

[dwrensha]

The RPC system depends heavily on single-threaded-ness. I doubt that you would be able to get away with just switching some Rc<RefCell<T>> to Arc<Mutex<T>>. Much more intrusive changes would likely be required. For example, rpc::Request would need to synchronize on any access to its ConnectionState. https://github.com/capnproto/capnproto-rust/blob/a9497907b8a13efe5362d84721637626230e851a/capnp-rpc/src/rpc.rs#L1737

Moreover, to makes things work out, we would probably also need to add a Send bound to ClientHook. But then there there's the question of what to do about user code that currently depends on single-threaded-ness. For example, PublisherImpl in the pubsub example uses Rc<RefCell<T>> in its internal state:

https://github.com/capnproto/capnproto-rust/blob/a9497907b8a13efe5362d84721637626230e851a/capnp-rpc/examples/pubsub/server.rs#L70-L73

If ClientHook requires Send, then this code would not be allowed. (I don't have a good sense of how many downstream users actually depend on being able to use non-Send data in RPC objects, but it's certainly a pattern that I have used a bunch.)

[b00f]

Let's address an important question first: Is it necessary to implement Sync for the RpcSystem?

I understand that the ability to spawn the RpcSystem is a crucial requirement for our users. It's important that we do not limit ourselves by the restrictions of the Rust programming language.

Regarding the potential need to add a Send bound to ClientHook, I don't think this should be a major concern. Upgrading from Non-Send to Send should not pose too much difficulty for our users.

[dwrensha]

Let's address an important question first: Is it necessary to implement Sync for the RpcSystem?

I don't understand exactly what you are asking here. RpcSystem is not Sync because it contains Rc data.

I understand that the ability to spawn the RpcSystem is a crucial requirement for our users.

By "spawn", do you mean "spawn on a separate thread"?

[b00f]

es, that's correct. "Spawn" in this context refers to the ability to run the RpcSystem on a separate thread. Due to the lack of Sync implementation, it's currently not possible to do so.

I understand that changing the code as previously suggested may not be straightforward and could lead to additional issues. It's good to consider alternative approaches to solving this problem. Let's see if we can find a solution that is both effective and feasible.

[dwrensha]

"Spawn" in this context refers to the ability to run the RpcSystem on a separate thread.

Can you explain more about why you want this? Some possible reasons I can think of are:

1. you are implementing RPC objects that use lots of CPU and you want to offload them from the main thread
2. you are using an async executor that requires its tasks to be Send

[b00f]

I updated the hello_world example in order to reproduce the issue. Look at here: https://github.com/capnproto/capnproto-rust/compare/master...b00f:capnproto-rust:master

The point is, client calls server, later server queries the client's name, then responses with "Hello {name}!".

[dwrensha]

That example works for me if I change the spawn to spawn_local:

$ git diff
diff --git a/capnp-rpc/examples/hello-world/server.rs b/capnp-rpc/examples/hello-world/server.rs
index 6db10a95..b4700043 100644
--- a/capnp-rpc/examples/hello-world/server.rs
+++ b/capnp-rpc/examples/hello-world/server.rs
@@ -59,7 +59,7 @@ impl hello_world::Server for HelloWorldImpl {
         
         let (tx, rx) = oneshot::channel();
         let provider = Provider { name_service };
-        tokio::task::spawn(async move {
+        tokio::task::spawn_local(async move {
             let name = provider.get_name().await;
             if let Err(_) = tx.send(name) {
                 println!("The receiving end of the channel has been dropped");

Is there a specific reason that you want to use spawn instead of spawn_local?

[b00f]

Is there a specific reason that you want to use spawn instead of spawn_local?

Ah, thank you for pointing out the difference between spawn and spawn_local. I was not aware that spawn_local is specifically for !Send futures. Your hint was extremely helpful. Based on this information, it seems that this issue can be considered resolved.