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

Decide on message API - allow multiple message types per actor? #11

Closed
mcginty opened this issue Feb 19, 2021 · 5 comments · Fixed by #38
Closed

Decide on message API - allow multiple message types per actor? #11

mcginty opened this issue Feb 19, 2021 · 5 comments · Fixed by #38
Assignees
Labels
API Relates to public API surface

Comments

@mcginty
Copy link
Collaborator

mcginty commented Feb 19, 2021

The original actor system has an API such that an actor accepts a single message type, causing us to often use enums to allow different types of messages to be sent a la:

enum MyMessage {
  Hello,
  Goodbye
}

impl Actor for MyActor {
  type Message = MyMessage;
}

Actix and other systems use something like:

impl Handler<Hello> for MyActor { ... }

impl Handler<Goodbye> for MyActor { ... }

Would be good to work out the pros/cons.

@strohel strohel added the API Relates to public API surface label Feb 19, 2021
@strohel strohel changed the title Decide on message API Decide on message API - allow multiple message types per actor? Feb 21, 2021
@strohel
Copy link
Member

strohel commented Feb 21, 2021

I haven't yet formed opinion on this, but some considerations:

  1. If multiple message types per actor are allowed, would that mean multiple (input) channels for a single actor?
    • if yes, this could even help with Support priority channels #22
    • if no, would this mean we would need to box (indirection) to marshal different types into a channel? Or perhaps somehow generate the enum? (that sounds like a proc macro)
    • From crossbeam::select! macro that we use:

      If multiple operations are ready at the same time, a random one among them is selected.

  2. Perhaps the need to create these enums is a reasonable price to pay for the simplicity of the crate?

@bschwind
Copy link
Member

This has actually been one of the biggest pain points for me in the current actor API. Let's say I have two actors, A and B.

A and B used to not communicate at all with each other, but suddenly a new requirement arises and I want to start sending messages from A to B. If B's message type used to just be a simple struct, I now I have to refactor that into an enum and add a variant so A can send the type it desires to B.

I then have to go through all instances in the code where other actors sent messages to B, and change them to use the new enum message.

Contrast this with the trait approach, where I simply impl Handler<NewMessage> for B, and suddenly actor A can send NewMessage to B.

would that mean multiple (input) channels for a single actor?

My intuition says "yes" here, especially if we wanted priority channels.

We're also not tied to crossbeam for channels, there are other implementations like flume which are worth investigating. We could possibly use something like the Selector struct to accomplish dynamic polling from multiple channels.

A procedural macro is another possibility but I'd leave that as more of a "last resort" solution.

@mcginty
Copy link
Collaborator Author

mcginty commented Feb 22, 2021

Yeah, I agree this situation of making it easier to receive multiple message types is one that needs to be solved cleanly in a new design, so either adding a Handler trait, or having some better typing in the sending pipeline so that you can still do:

struct Hello;
struct Goodbye;

enum MyMessage {
  Hello(Hello),
  Goodbye(Goodbye)
}

impl From<Hello> { ... }
impl From<Goodbye> { ... }

impl Actor for MyActor {
  type Message = MyMessage;
  ...
}

so that you can eventually do something like:

self.my_actor.send(Goodbye)?;

The downside of the above approach that we've been using is that any struct that wants to hold a Recipient this way needs to have a signature like this now:

pub struct OtherActor<M: From<Goodbye>> {
    next: Recipient<M>,
}

which is kind of gross looking a lot of the time, especially when you are holding multiple recipients in the same struct. If there's some type magic that makes that pattern cleaner, I'd say it's also a possible solution that would make it more ok to keep the enum approach.

@strohel strohel added this to the Before Publish milestone Feb 22, 2021
@strohel strohel self-assigned this Feb 22, 2021
@strohel strohel modified the milestone: Before Publish Feb 22, 2021
@strohel
Copy link
Member

strohel commented Feb 22, 2021

Some research into how Actix does things:

  • It requires all messages to implement Message (which is just trait Message { type Result }), but this is AFAICS not a prerequisite of its support for multiple handlers per actor.
  • its Address::recipient() method has M: Message (local) template param (bound by the handlers that the actor supports). I.e. multiple differently typed Recipients can be created out of a single Address. This is an inspiration for type-level tricks.
  • it uses its own implementation of a MPSC queue which is reportedly mostly copy of the one from stdlib, combined (somehow?) with tokio oneshot::channel created for each message.
  • it uses a single queue for each actor (for multiple message types) by boxing each message into Envelope.

@strohel
Copy link
Member

strohel commented Feb 22, 2021

Some very quick research into flume:

  • its API and semantics are very close to that of crossbeam-channel.
  • its Selector and crossbeam's Select seem to be also similar. flume's one has simpler, but possibly more constrained API - though sufficient for us AFAICS. (in flume, all handlers for different selected messages must return the same T. In crossbeam, one handles received messages more elaborately/flexibly)
  • if flume's evantual-fairness feature is not enabled, its Selector seems to poll the passed channels in deterministic fashion (hint for message priorities).
  • hint for possible async support Support for actors expressed as async/await fuctions? #13: flume has optional, but direct support for async adaptors. for crossbeam there is channel-async, but it says right in its readme:

    Due to interactions between crossbeam and tokio, it is recommended that you use the channels in async-std

strohel added a commit that referenced this issue Feb 24, 2021
This is to erase `M` generic parameter from Recipient method return types,
which may help me with #11.

Does a bit of what #20 wants to do, consider this commit temporary until
I can rebase.
strohel added a commit that referenced this issue Feb 24, 2021
…not_full() differently

I expect that at least the second point will be resolved (more correctly) by #20.

Not sure whether we want to keep Recipient::remaining_capacity() or whether
it will be eventually superseded by #22.

This a temporary commit just to make prototyping of #11 easier.
strohel added a commit that referenced this issue Feb 24, 2021
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)
strohel added a commit that referenced this issue Feb 24, 2021
...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 in defining a `Sender<M>` trait to abstract crossbeam::Sender,
implementing `<M: Into<N>> Sender<M>` for `crossbeam::Sender<N>`, and boxing
*that* implementation in Addr::recipient().

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.

There was a lot of design choices with subtle differences (like making
Recipient a trait), this change preferred ones that meant a smaller change.
strohel added a commit that referenced this issue Feb 25, 2021
This is to erase `M` generic parameter from Recipient method return types,
which may help me with #11.

Does a bit of what #20 wants to do, consider this commit temporary until
I can rebase.
strohel added a commit that referenced this issue Feb 25, 2021
…not_full() differently

I expect that at least the second point will be resolved (more correctly) by #20.

Not sure whether we want to keep Recipient::remaining_capacity() or whether
it will be eventually superseded by #22.

This a temporary commit just to make prototyping of #11 easier.
strohel added a commit that referenced this issue Feb 25, 2021
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)
strohel added a commit that referenced this issue Feb 25, 2021
...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 in defining a `Sender<M>` trait to abstract crossbeam::Sender,
implementing `<M: Into<N>> Sender<M>` for `crossbeam::Sender<N>`, and boxing
*that* implementation in Addr::recipient().

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.

There was a lot of design choices with subtle differences (like making
Recipient a trait), this change preferred ones that meant a smaller change.
strohel added a commit that referenced this issue Feb 25, 2021
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.
strohel added a commit that referenced this issue Feb 25, 2021
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)
strohel added a commit that referenced this issue Feb 25, 2021
...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 added a commit that referenced this issue Feb 26, 2021
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.
strohel added a commit that referenced this issue Feb 26, 2021
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)
strohel added a commit that referenced this issue Feb 26, 2021
...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 added a commit that referenced this issue Feb 26, 2021
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.
strohel added a commit that referenced this issue Feb 26, 2021
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)
strohel added a commit that referenced this issue Feb 26, 2021
...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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Relates to public API surface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants