Timed infinite loop fix #80

strohel · 2024-01-12T21:19:57Z

timed tests: rename test, assert only after shutdown, extend timeline comments

Add test for issue #79

Not currently failing, just demonstrates the unfortunate behavior.

timed: fix #79 by enqueueing (rather than handling) delayed messages 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 self-sending messages never handling any delayed/recurring messages (Delayed/recurring messages are not delivered to busy actors #72)
actors with recurring messages that are slower to handle than their interval eventually not processing any outside messages (Inifinite loop when Timed actor's handle() is slower than recurring message interval #79)

See the tweaked tests for the change of behavior.

Checking how the test results change commit-by-commit should give a good story.

… comments

Not currently failing, just demonstrates the unfortunate behavior.

…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.

bschwind

LGTM, though I have a small question, and a spelling fix.

bschwind · 2024-01-15T08:15:22Z

src/timed.rs

@@ -301,38 +303,78 @@ mod tests {
                context.myself.send_now(3).unwrap();
            }

-            Ok(())
-        }
+            // Message 2 is a recurring one, sleep based on a paremeter.


Suggested change

// Message 2 is a recurring one, sleep based on a paremeter.

// Message 2 is a recurring one, sleep based on a parameter.

bschwind · 2024-01-15T08:21:01Z

src/timed.rs

+            // Recurring and Delayed messages are only added to the queue when handled, and then go
+            // through actors priority inboxes again when actually enqueued.


I'm having trouble visualizing the message flow based on this comment.

only added to the queue when handled

In my mind, "handled" means the actor has run the handle() function on the message, so it's already out of the queue.

Is it possible to be more specific in the language here about the flow of the message from when send_delayed is called to when it actually ends up in the actor handle() function?

Agree. What about

Suggested change

// Recurring and Delayed messages are only added to the queue when handled, and then go

// through actors priority inboxes again when actually enqueued.

// These priorities apply to the set-up of Delayed and Recurring messages and we want to handle that pronto.

// The resulting inner message then comes back as `Instant` and is prioritized per its internal priority.

@goodhoko you suggestion reads very good to me using it (with very small tweaks)

goodhoko

Thanks for materialising our long discussion into an actual fix @strohel!

Behaviorally this looks all correct! I only have some suggestion for improving the comments. Let's give them good amount of attention as they're important for understanding the convoluted logic of Timed wrapper. .)

src/timed.rs

goodhoko · 2024-01-15T11:42:18Z

src/timed.rs

+            // Recurring and Delayed messages are only added to the queue when handled, and then go
+            // through actors priority inboxes again when actually enqueued.


Agree. What about

Suggested change

// Recurring and Delayed messages are only added to the queue when handled, and then go

// through actors priority inboxes again when actually enqueued.

// These priorities apply to the set-up of Delayed and Recurring messages and we want to handle that pronto.

// The resulting inner message then comes back as `Instant` and is prioritized per its internal priority.

strohel · 2024-01-16T21:48:43Z

@bschwind @goodhoko I've pushed a fixup commit that extends comments as suggested (+ also extends the docs directly on TimedMessage variants). PTAL.

goodhoko

This is perfect! Especially the doc comments on TimedMessage variants are very helpfull. Good to merge IMO!

I suppose this can be released with a mere patch bump. Or do we consider the change in delivery order to be breaking?

strohel · 2024-01-17T11:05:36Z

I suppose this can be released with a mere patch bump. Or do we consider the change in delivery order to be breaking?

Good point. The rename of the TimedMessage::Delayed fire_at field to enqueue_at is technically semver-incompatible, so we should bump the minor version. It will at make it easier for donwstreams to downgrate actor version shuold they need it.

I'll prepare portal version that uses this PR before merging to get some real-life testing. It doesn't technically require any code changes (the renamed fields are not directly used). Also opportunity for @bschwind to check clarity of comments of the latest version.

goodhoko · 2024-01-17T13:32:21Z

The rename of the TimedMessage::Delayed fire_at field to enqueue_at is technically semver-incompatible

Ahaaa! I assumed that the enum field is private by default (as are struct fields) but actually enum variants and their fields always share the visibility of the enum itself. I.e. the field is part of the crate's public API.

so we should bump the minor version. It will at make it easier for donwstreams to downgrate actor version shuold they need it.

Per the semver spec, if it's backward incompatible we should bump the major version. Minor version bumps are for additions (backward compatible changes). It feels excessive but hey, it's just a version number.

strohel · 2024-01-17T13:44:05Z

Per the semver spec, if it's backward incompatible we should bump the major version. Minor version bumps are for additions (backward compatible changes). It feels excessive but hey, it's just a version number.

Yup, but 0.x.y versions are special, in both semver and cargo's semantics https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-dependencies-from-cratesio

0.x.y means "pre-production" in both. actor is arguably production-ready now, though I don't feel like doing that step (of updating from 0.x to 1.0) now
SemVer says e.g. 0.3.0 and 0.3.1 are never compatible
cargo says that 0.3.1 can be used when "0.3.0" or "0.3" are specified (patch version backward compatibility)
- 0.3 and 0.4 are always incompatible even with cargo (while 1.4 can be used when 1.3 is requsted)

goodhoko · 2024-01-17T13:46:19Z

@strohel Got it. I didn't realize actor is still pre-1.0. Thanks!

strohel added 3 commits January 12, 2024 21:26

timed tests: rename test, assert only after shutdown, extend timeline…

60c123d

… comments

Add test for issue #79

e6fb7df

Not currently failing, just demonstrates the unfortunate behavior.

strohel requested review from bschwind, PabloMansanet and goodhoko January 12, 2024 21:19

bschwind approved these changes Jan 15, 2024

View reviewed changes

goodhoko approved these changes Jan 15, 2024

View reviewed changes

strohel force-pushed the timed-infinite-loop-fix branch from 6e473f3 to 294b065 Compare January 16, 2024 21:50

Fix/extend comments based on review + elsewhere

b4b0bc2

strohel force-pushed the timed-infinite-loop-fix branch from 294b065 to b4b0bc2 Compare January 16, 2024 21:50

goodhoko approved these changes Jan 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timed infinite loop fix #80

Timed infinite loop fix #80

strohel commented Jan 12, 2024 •

edited by goodhoko

Loading

bschwind left a comment

bschwind Jan 15, 2024

bschwind Jan 15, 2024

goodhoko Jan 15, 2024 •

edited

Loading

strohel Jan 16, 2024

goodhoko left a comment

goodhoko Jan 15, 2024 •

edited

Loading

strohel commented Jan 16, 2024 •

edited

Loading

goodhoko left a comment

strohel commented Jan 17, 2024

goodhoko commented Jan 17, 2024 •

edited

Loading

strohel commented Jan 17, 2024

goodhoko commented Jan 17, 2024

	// Message 2 is a recurring one, sleep based on a paremeter.
	// Message 2 is a recurring one, sleep based on a parameter.

		// Recurring and Delayed messages are only added to the queue when handled, and then go
		// through actors priority inboxes again when actually enqueued.

Timed infinite loop fix #80

Are you sure you want to change the base?

Timed infinite loop fix #80

Conversation

strohel commented Jan 12, 2024 • edited by goodhoko Loading

timed tests: rename test, assert only after shutdown, extend timeline comments

Add test for issue #79

timed: fix #79 by enqueueing (rather than handling) delayed messages when due

bschwind left a comment

Choose a reason for hiding this comment

bschwind Jan 15, 2024

Choose a reason for hiding this comment

bschwind Jan 15, 2024

Choose a reason for hiding this comment

goodhoko Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

strohel Jan 16, 2024

Choose a reason for hiding this comment

goodhoko left a comment

Choose a reason for hiding this comment

goodhoko Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

strohel commented Jan 16, 2024 • edited Loading

goodhoko left a comment

Choose a reason for hiding this comment

strohel commented Jan 17, 2024

goodhoko commented Jan 17, 2024 • edited Loading

strohel commented Jan 17, 2024

goodhoko commented Jan 17, 2024

strohel commented Jan 12, 2024 •

edited by goodhoko

Loading

goodhoko Jan 15, 2024 •

edited

Loading

goodhoko Jan 15, 2024 •

edited

Loading

strohel commented Jan 16, 2024 •

edited

Loading

goodhoko commented Jan 17, 2024 •

edited

Loading