tonarino / actor

A minimalist actor framework aiming for high performance and simplicity.
MIT License
39 stars 6 forks source link

Support creating differently-typed recipients in Addr::recipient() #38

Closed strohel closed 3 years ago

strohel commented 3 years ago

...within bounds that M: Into for Recipient for Actor.

Fixes #11 (in its narrow sense) without creating multiple channels per actor and without boxing the messages transferred.

Has been made possible by removal of the ability to retrieve back the failed-to-send message in one of the earlier commits.

The whole trick is to box crossbeam Sender<M> into Arc<dyn SenderTrait<M>> in Receiver, and implementing SenderTrait<M> for crossbeam Sender and for boxed version of itself (!), second time with ability to convert M.

All generic parameters in the echo example have disappeared without losing any flexibility, yay!

Speaking strict semver, this is an API-breaking change, but e.g. the media_pipeline compiles and works unchanged.

v2: make the change smaller and simpler by not messing up with GenericReceiver. Enables nicer API and keeping SenderTrait private, at the small expense of one extra boxing when creating Addr, and one extra pointer indirection when calling send() family of functions. All that thanks to @skywhale's clever question.


I suggest reviewing by commits and not squashing during merge, the main one if the last one.

strohel commented 3 years ago

I've force-pushed v2 of the change.

Thanks to @skywhale's clever question, I was able make the change smaller and simpler by not messing up with GenericReceiver. That enables nicer API and keeping SenderTrait (renamed from Sender) private. The price for that is one extra boxing ("arc'ing"?) when creating Addr (not frequent), and one extra pointer indirection when calling the send() family of functions (should be negligible compared to sending message across threads).

I've also removed the temporary commit (so that send_if_not_full() is kept for now), and polished the commit adding SendError.

This should make this PR merge-ready (I suggest not squashing commits during merge).

strohel commented 3 years ago

As @bschwind said, the only compromise of this approach vs. impl Handler is that we can't have different channels per message type to have different priorities per type.

Right, this does not resolve #22 along the way. My reasoning was, thinking about the future state with priorities implemented:

API-wise, do we want to bind the priority to the message type, or shall we let the sender decide, i.e. bind the priority to the message instance? The sender-decided priority is functionally a superset of message-based priority. That alone doesn't make it a better choice, but I haven't found strong reasons to make the API more opinionated either (thoughts welcome). I've basically pushed that decision to the point when #22 is implemented, though this currently leans on the sender-decided priority side (that was simply a smaller change).

I think that #22 shall be relatively straightforward to implement if we start with a small fixed set of priorities (Normal, High perhaps). I.e. that it will be possible to circumvent the "collect message types (priorities) from trait impls. and create channel for each of them in Receiver" problem (which may or may not be solvable) there that same way as I've done here.

Now, does this make sense to somebody who's not me? :)