tonarino / actor

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

Implement message receive timeout, the Actor::deadline_passed() callback #53

Closed strohel closed 3 years ago

strohel commented 3 years ago

This is a result of an experimentation triggered by #52, and relates itself to #14.

Direction-wise this is an alternative to #52, but this PR does not implement a full-fledged timer, merely a building block on top of which one can be made. Simplified timer is presented as an example.

The main differentiating point is a focus on minimal additions to the core actor crate by allowing downstreams to implement their own in form of an Actor (though we can ship some ourselves). The design should allow implementing timers without any permanent "ticks", with resolution of thread::park_timeout() used by crossbeam-channel and ability to be interrupted by an incoming message.

From the API user PoV, the difference is that with this approach, the user has the burden of carrying some Recipient<TimerActor> around, while in #52, tonari-actor itself has to stick TimerRef somewhere.

The first 2 commits are preparatory ones (first extracted from Brian's PR), Implement Context::set_deadline() and Actor::deadline_passed() callback is the main one.

skywhale commented 3 years ago

I didn't realize that crossbeam-channel provides timer functionalities. Good to know :)

@bschwind, as far as I know, all the actors who send messages themselves handle only one fixed-interval cycle. In that case, using @strohel's framework, would the following be sufficient?

impl Actor for SomeActor {
  fn started() {
    context.set_deadline(Instant::now());
  }
  fn deadline_passed(...) {
    context.myself.send(Message::Check);
    context.set_deadline(Instant::now() + CHECK_INTERVAL);
  }
  fn handle(...) {
    match message {
      Message::Check => {
        do_some_recurring_stuff();
      },
    }
  }
}

or even:

impl Actor for SomeActor {
  fn started() {
    context.set_deadline(Instant::now());
  }
  fn deadline_passed(...) {
    do_some_recurring_stuff()
    context.set_deadline(Instant::now() + CHECK_INTERVAL);
  }
}

When we have a more complex actors who have multiple recurring cycles at different intervals, then we can use something along the line of DelayActor example

strohel commented 3 years ago

Similarly to @skywhale, I was also thinking that for a "actor with tick" use-case, we don't even need a separate actor/thread if we have the Context::set_deadline().

(A) I've come up with a TickingActor + TickingWrapper to share code among actors that need ticks:

Code for TickingActor and TickingWrapper ```rust /// An actor with a periodic tick. Wrap using [TickingWrapper] to activate the ticks. trait TickingActor: Actor { const PERIOD: Duration; /// Callback that is called every [Self::PERIOD] as long as this actor is running. fn tick(&mut self, context: &mut Context); } struct TickingWrapper { pub inner: A, started_at: Instant, } impl TickingWrapper { fn schedule_tick(&mut self, context: &mut Context) { let elapsed_time = Instant::now() - self.started_at; // Nightly has https://doc.rust-lang.org/std/time/struct.Duration.html#method.div_duration_f64 let elapsed_ticks = (elapsed_time.as_secs_f64() / A::PERIOD.as_secs_f64()).floor(); let deadline = self.started_at + A::PERIOD.mul_f64(elapsed_ticks + 1.0); context.set_deadline(deadline); } } impl Actor for TickingWrapper { type Error = A::Error; type Message = A::Message; fn name() -> &'static str { A::name() } fn handle( &mut self, context: &mut Context, message: Self::Message, ) -> Result<(), Self::Error> { self.schedule_tick(context); self.inner.handle(context, message) } fn started(&mut self, context: &mut Context) { self.started_at = Instant::now(); self.schedule_tick(context); self.inner.started(context); } fn stopped(&mut self, context: &mut Context) { self.inner.stopped(context); } fn deadline_passed(&mut self, context: &mut Context) { self.schedule_tick(context); self.inner.tick(context); } } ```

The code doesn't currently compile though: the presence of Context<Self> in Actor::handle() and friends basically prevents us from forwarding handle() to inner actor. But it seems on the first look that we can change Context to be Context<Self::Message>, i.e.

-pub struct Context<A: Actor + ?Sized> {
+pub struct Context<M> {
    pub system_handle: SystemHandle,
-    pub myself: Addr<A>,
+    pub myself: Recipient<M>,
    recv_deadline: Option<Instant>,
}

That would enlarge the scope though.

(B) Another possibility would be to implement something similar using composition rather than as a wrapper: introduce some TickHelper that actors could add as a field, but then these actors would have to remember to call tick_helper.schedule_tick(context) in their started(), handle() and deadline_passed(), which looks fragile.

(C) Yet another way would be to implement support for such periodic ticks right into tonari-actor core. Note that simply setting the timeout to some constant Actor::PERIOD in run_actor_select_loop() won't produce a steady tick, because incoming messages would reset the timeout. Some form of computation analogous to one in TickingWrapper would be needed. That is mostly the reason why I originally suggested the deadline to be one-shot rather than persistent. But that can be revisited. BTW, crossbeam-channel has a more suitable API that accepts deadline: Instant rather than timeout: Duration, but that doesn't seem to be compatible with the select! convenience macro.

I'm very lightly leaning towards (A), less to (B) and less to (C), but such minimalism may end up being impractical, so not at all strong opinion here.

In this comment I've dumped everything I had in my sleeves, so @bschwind feel free to iterate on the ergonomic API on top of this, either in a new branch on top or by pushing commits here.

strohel commented 3 years ago

My head only produces new ideas slowly: I've pushed one extra commit that changes the timeout to be queried from the loop, rather than set by the actor through context.

It seems better to me, and should make the approach (B) in my previous message more feasible. We also no longer need to pass Context as &mut (but that is not changed yet in the PR).

bschwind commented 3 years ago

@strohel thanks for your research on this! I'll try integrating this branch into tonari along with @skywhale 's code suggestion above. If it works well we can try fixing up the API a bit more to include a convenience wrapping actor, and/or a more ergonomic API on top of it all.

The most recent commit likely makes the implementation of a wrapping actor easier, but now it's a bit tougher to implement actors which send recurring messages to themselves. For example, if an actor sends a recurring message with a delay of 0 and a recurrence of 1 second, I would have to store state on the actor to know that the first return value of receive_timeout should be 0 and subsequent calls should return a duration of 1 second.

skywhale commented 3 years ago

I think (A) makes the most sense, if we can change the semantics of deadline_passed a bit, so it's not reset by incoming messages. Otherwise, I think the API is a little tricky to use. I like the flexibility of the original set_deadline and deadline_passed, assuming the changed semantics. Not to mention TickingWrapper, users can easily implement various types of timing logic.

(A) not as accurate as TickingWrapper, but something as simple as this can implement approximate ticks.

fn deadline_passed() {
  context.set_deadline(INTERVAL);
  do_recurring_stuff();
}

(B) if do_recurring_stuff() is an expensive call that take a few milliseconds, this pattern may be more appropriate.

fn deadline_passed() {
  do_recurring_stuff();
  context.set_deadline(INTERVAL);
}
strohel commented 3 years ago

@bschwind, @skywhale, thank you for your feedback. It made me understand that setting (rather than querying) the deadline could indeed be a more ergonomic API, and that we can preserve the deadline even when a message is received.

I've force-pushed a new version that incorporates both ideas and is rebased on main. The first 3 commits are preparatory and partially revert the previous implementation, the fourth one is the actual functionality. So the overall PR diff shows the difference between the 2 implementations and usage in the timer example, and the "pure additions" can be seen by looking at the last commit in isolation.

strohel commented 3 years ago

As the next step, it's helpful to land #54 and provide an actor wrapper that implements a similar API to @bschwind's sendrecurring* functions. Assuming it's a popular use case of the actor system, we can put it in a feature-gated util mod instead of an example.

Is the assumption right now that we'll add in some built-in convenience actors (perhaps under a feature flag as @skywhale mentioned) in a separate followup PR?

Right, I count with opening a follow-up PR to extend and promote DelayActor, possibly after landing #54 as Ryo suggested.

I've pushed new commit that should address most review points above (thanks for them). I've resolved relevant conversations. (do you have a preferred convention who resolves/when to resolve them?)