Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Feb 26, 2021

Conversation

strohel
Copy link
Member

@strohel strohel commented Feb 24, 2021

...within bounds that M: Into for Recipient for Actor<Message = N>.

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.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Base automatically changed from error-types to main February 25, 2021 05:50
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@strohel
Copy link
Member Author

strohel commented Feb 25, 2021

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 strohel marked this pull request as ready for review February 25, 2021 22:39
Copy link
Collaborator

@mcginty mcginty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me! Nice type magic :).

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

src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@skywhale skywhale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great :)

src/lib.rs Show resolved Hide resolved
This is to erase `M` generic parameter from Recipient method *return* types,
which is a prerequisite of type system-based solution of #11.

As a side-effect, this also implements one prerequisite step for #20.
I.e before this change, Input, Damper actors assumed they are fed into Mixer,
but Delay assumed it is fed into something that expects naked `Chunk`.
This made no sense, and made actors dependent on how they are wired in the
system, which is an antipattern.

This is an artificial change that demonstrates problems outlined in
#11 (comment)
Thanks to the previous commit, this is now possible to do this
_without touching the actors themselves._

Artificial change for demonstration purposes.

This brings no change functionality-wise.
...within bounds that that M: Into<N> for Recipient<M> for Actor<Message = N>.

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.
@strohel strohel merged commit 04ca99f into main Feb 26, 2021
@strohel strohel deleted the message-api-11 branch February 26, 2021 09:06
@strohel
Copy link
Member Author

strohel commented Feb 26, 2021

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? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decide on message API - allow multiple message types per actor?
3 participants