ractor icon indicating copy to clipboard operation
ractor copied to clipboard

API improvement

Open George-Miao opened this issue 1 year ago • 1 comments

While using ractor, I found some inconvinience while dealing with the API, especially DiscardHandler and WorkerBuilder. Is it possible to impl those two traits for closures? For now, I have these shim code:

fn worker_builder(&self) -> Box<dyn WorkerBuilder<SourceWorker>> {
        struct SourceWorkerBuilder {
            db: Collection<Record>,
        }

        impl WorkerBuilder<SourceWorker> for SourceWorkerBuilder {
            fn build(&self, id: WorkerId) -> SourceWorker {
                SourceWorker {
                    id,
                    db: self.db.clone(),
                }
            }
        }

        Box::new(SourceWorkerBuilder {
            db: self.db.clone(),
        })
    }

and when initializing:

let builder = self.worker_builder();
Actor::spawn(None, factory, builder).await?;

which if we can rewrite it with closure:

let builder = Box::new({
  let db = self.db.clone(); 
  move |id| SourceWorkerBuilder { id, db: db.clone() }
})
Actor::spawn(None, factory, builder).await?;

Also, it might be good if we can use

pub fn where_is<K: Borrow<ActorName>>(name: K) -> Option<ActorCell>

Which should work no matter what ActorName is because underlying API provided by DashMap.

George-Miao avatar Apr 28 '23 04:04 George-Miao

So I "think" this is possible for WorkerBuilder (though it will be an API break), but for DiscardHandler I don't think you can clone closures can you? Especially if they're boxed, and because discards can happen on a worker OR by the factory (depending on the routing mode), you need to be able to clone the implementation (hence the clone_box(..) function).

For WorkerBuilder only the factory needs a handle to it, so that should be relatively easy. I don't have a lot of bandwidth to try this and rewrite the tests, but you're welcome to put up a PR. Otherwise we'll schedule it for a future release probably when we're doing another major change anyways.

slawlor avatar Aug 19 '23 15:08 slawlor