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

Modular Context #91

Closed
thomaseizinger opened this issue Jun 21, 2022 · 9 comments · Fixed by #133
Closed

Modular Context #91

thomaseizinger opened this issue Jun 21, 2022 · 9 comments · Fixed by #133

Comments

@thomaseizinger
Copy link
Collaborator

Without having a better name for this idea, here is what this is about:

With #85, the eventloop in Context is becoming very simple. It would amazing if you could polish up the APIs around Sender, ActorMessage etc in such a way that we can expose all these types publicly and provide them as fundamental building blocks for an actor system, together with a "basic" event loop that just reads and dispatches messages.

Anything on top like instrumentation, logging or things like #41 could then be solved outside of this library.

This was referenced Jun 21, 2022
@thomaseizinger
Copy link
Collaborator Author

thomaseizinger commented Jun 21, 2022

Here is an initial draft:

  • Move notify-interval and notify-after to SendFuture as repeat-every and delay
  • Provide public next_message function on Context
  • Wrap ActorMessage in an opaque type that has a single handle(self, actor: &mut Actor) -> Result<(), ActorShutdown> function
  • Have next_message return this opaque type
  • Make run, select and join free functions. They can now be implemented on top of Context::next_message
  • Remove Address::attach_stream: It is redundant with StreamExt::forward
  • Do something like Type-safe actor lifecycle #76

@Restioson
Copy link
Owner

Move notify-interval and notify-after to SendFuture as repeat-every and delay

This could maybe be done in an extension crate too

Wrap ActorMessage in an opaque type that has a single handle(self, actor: &mut Actor) -> Result<(), ActorShutdown> function

How would this pass the context to the actor's Hander::handle?

Make run, select and join free functions

What is the advantage of making them free functions over functions of Context?

They can now be implemented on top of Context::next_message

In theory, they could also be in an extension crate. run probably should be in the main crate, though.

@thomaseizinger
Copy link
Collaborator Author

Move notify-interval and notify-after to SendFuture as repeat-every and delay

This could maybe be done in an extension crate too

Depending on how much type-safety we want, I am not sure it can. e.g. I think repeat_every should enforce Return = ().

Wrap ActorMessage in an opaque type that has a single handle(self, actor: &mut Actor) -> Result<(), ActorShutdown> function

How would this pass the context to the actor's Hander::handle?

I was thinking of next_message to pass a mutable borrow of Context into the opaque type.

Make run, select and join free functions

What is the advantage of making them free functions over functions of Context?

It makes it clear what the minimal public API of xtra is and by having them as free-functions, we eat our own dogfood and can't access internals.

They can now be implemented on top of Context::next_message

In theory, they could also be in an extension crate. run probably should be in the main crate, though.

True.

@Restioson
Copy link
Owner

Restioson commented Jun 22, 2022

Depending on how much type-safety we want, I am not sure it can. e.g. I think repeat_every should enforce Return = ().

#[async_trait]
trait RepeatExt {
    async fn repeat_every(interval: Duration);
}

#[async_trait]
impl<F, TResolveMarker> RepeatExt for SendFuture<(), F, TResolveMarker>
    where F: ...
{
    async fn repeat_every(interval: Duration) {
        // body
    }
}

Could work I think. Async trait is only used for brevity - a combinator could be used instead.

It makes it clear what the minimal public API of xtra is and by having them as free-functions, we eat our own dogfood and can't access internals.

I am not sure free functions accomplish this, as they can still be implemented with crate internal features through pub(crate).

@thomaseizinger
Copy link
Collaborator Author

Depending on how much type-safety we want, I am not sure it can. e.g. I think repeat_every should enforce Return = ().

#[async_trait]
trait RepeatExt {
    async fn repeat_every(interval: Duration);
}

#[async_trait]
impl<F, TResolveMarker> RepeatExt for SendFuture<(), F, TResolveMarker>
    where F: ...
{
    async fn repeat_every(interval: Duration) {
        // body
    }
}

Could work I think. Async trait is only used for brevity - a combinator could be used instead.

I am also happy to remove the functions for now and introduce an extension crate. I just figured that adding them to the core SendFuture implementation is easier to start with!

It makes it clear what the minimal public API of xtra is and by having them as free-functions, we eat our own dogfood and can't access internals.

I am not sure free functions accomplish this, as they can still be implemented with crate internal features through pub(crate).

It doesn't prevent it but cheating with pub(crate) is a lot more obvious compared to accessing private, inner fields of Context. Why would you like to keep them on Context if they don't have to be there?

@Restioson
Copy link
Owner

Why would you like to keep them on Context if they don't have to be there?

context.select(self, fut) seems more ergonomic than select(context, self, fut) to me 🤷

@thomaseizinger
Copy link
Collaborator Author

thomaseizinger commented Jun 22, 2022

Why would you like to keep them on Context if they don't have to be there?

context.select(self, fut) seems more ergonomic than select(context, self, fut) to me 🤷

Hmm, matter of taste I guess? futures::future::select is also a free-function and not future1.select(future2);

I guess the reason I'd prefer a free function is because the real goal here is to improve the encapsulation of Context. Once the public API of Context allows to express functions like run, select and join, they are effectively decoupled from the internals and as such, no longer need to live on Context. Typically, functions are only defined for structs because they need to access internal state to function.

FWIW I would probably call it like this:

xtra::select(context, fut, self).await;
  1. Calling it via the crate name saves an import and makes it unambiguous with futures::select without checking the use block
  2. context1 and fut are like the two futures passed to futures::select and should thus come first.

Footnotes

  1. I considered proposing a rename of Context to Mailbox but it seems a bit weird to pass a &mut Mailbox to each handler. Also stop_self would have to be renamed to close or something like that and I think it is not immediately clear that the actor will stop as a result.

@thomaseizinger
Copy link
Collaborator Author

We could define select to require a NextMessage future that is obtained via context.next_message() so it is more obvious that we are selecting between two futures?

let next_message = context.next_message();
xtra::select(next_message, fut, self).await;

That is what select would have to do internally but it is a little more verbose for callers. Maybe okay because it is a pretty niche function anyway?

@thomaseizinger thomaseizinger linked a pull request Jul 16, 2022 that will close this issue
2 tasks
@thomaseizinger
Copy link
Collaborator Author

Closing this in favor of #126.

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 a pull request may close this issue.

2 participants