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

Inifinite loop when Timed actor's handle() is slower than recurring message interval #79

Open
strohel opened this issue Dec 11, 2023 · 0 comments · May be fixed by #80
Open

Inifinite loop when Timed actor's handle() is slower than recurring message interval #79

strohel opened this issue Dec 11, 2023 · 0 comments · May be fixed by #80
Assignees

Comments

@strohel
Copy link
Member

strohel commented Dec 11, 2023

Caused by this code

    /// Process any pending messages in the internal queue, calling wrapped actor's `handle()`.
    fn process_queue(&mut self, context: &mut <Self as Actor>::Context) -> Result<(), A::Error> {
        // Handle all messages that should have been handled by now.
        let now = Instant::now();
        while self.queue.peek().map(|m| m.fire_at <= now).unwrap_or(false) {
            let item = self.queue.pop().expect("heap is non-empty, we have just peeked");

            let message = match item.payload {
                Payload::Delayed { message } => message,
                Payload::Recurring { mut factory, interval } => {
                    let message = factory();
                    self.queue.push(QueueItem {
                        fire_at: item.fire_at + interval,
                        payload: Payload::Recurring { factory, interval },
                    });
                    message
                },
            };

            // Let inner actor do its job.
            //
            // Alternatively, we could send an `Instant` message to ourselves.
            // - The advantage would be that it would go into the queue with proper priority. But it
            //   is unclear what should be handled first: normal-priority message that should have
            //   been processed a while ago, or a high-priority message that was delivered now.
            // - Disadvantage is we could easily overflow the queue if many messages fire at once.
            self.inner.handle(&mut TimedContext::from_context(context), message)?;
        }

        Ok(())
    }

Combined with the fact that we call this from handle().

I think that we should:

  • send message to ourselves instead of calling inner.handle(), so that it goes through the actor loop and gets proper priority
  • only process one message at a time: change while let to if let
    • this would resolve the concern in the TODO in the code that we

CC @goodhoko.

@strohel strohel self-assigned this Dec 11, 2023
strohel added a commit that referenced this issue Jan 12, 2024
Not currently failing, just demonstrates the unfortunate behavior.
strohel added a commit that referenced this issue Jan 12, 2024
…when due

Also rename `fire_at` to `enqueue_at` to be explicit about the fact.

This is a trade-off that prevents 2 sorts of bad behavior:
- actors with send-sending messages never handling any delayed/recurring messages (#72)
- actors with recurring messages that are slower to handle than their interval eventually not processing any outside messages (#79)

See the tweaked tests for the change of behavior.
@strohel strohel linked a pull request Jan 12, 2024 that will close this issue
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.

1 participant