Mitigate timer job submission failures #4852
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
so it is not necessary to prune them!
2 tasks
|
I think you'll need to explain this to me... |
crusso
reviewed
Jan 16, 2025
crusso
reviewed
Jan 16, 2025
| if (failed == 0) @timers := @prune @timers; | ||
| failed += 1; | ||
| @timers := ?(switch @timers { | ||
| case (?{ id = 0; pre; post; job = j; expire; delay }) |
There was a problem hiding this comment.
why does this match on id = 0?
There was a problem hiding this comment.
Only already reinsert-ed nodes (special id = 0) should be delegated into the pre part of the PQ.
There was a problem hiding this comment.
This tree rotation maintains the original expiration order.
ggreif
commented
Jan 17, 2025
| @@ -544,7 +544,7 @@ func @prune(n : ?@Node) : ?@Node = switch n { | |||
| if (n.expire[0] == 0) { | |||
| @prune(n.post) // by corollary | |||
| } else { | |||
| ?{ n with pre = @prune(n.pre); post = @prune(n.post) } | |||
| ?{ n with pre = @prune(n.pre) } | |||
There was a problem hiding this comment.
rationale: current node is not expunged, so the post portion won't contain any either
review feedback
ggreif
commented
Jan 17, 2025
| func reinsert(job : () -> async ()) { | ||
| if (failed == 0) { | ||
| @timers := @prune @timers; | ||
| ignore (prim "global_timer_set" : Nat64 -> Nat64) 1 |
There was a problem hiding this comment.
At this point there are no id = 0 nodes in the PQ. They were all in the front and have been gathered, expunged and pruned.
crusso
approved these changes
Jan 17, 2025
ggreif
added
automerge-squash
mergify bot
pushed a commit
that referenced
this pull request
Jan 17, 2025
This fixes a disappearing timer situation described by Timo in https://dfinity.slack.com/archives/CPL67E7MX/p1736347600078339. It turns out that under high message load the `async` timer servicing routine cannot be run. The fix is simple, check if the self-call succeeded (causes a `throw` already), and if not, set a very near global timer to retry ASAP (in the top-level `catch`). TODO: - [x] `catch` send errors for user workers (and mitigate) — see #4852 - [ ] document that the user thunk may be called more than once, and thus should have no side effects other than submitting the self-call — see dfinity/motoko-base#682
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This deals with the (unlikely) possibility that the send queue is not full when the timer servicing action is submitted, but becomes full while submitting the user jobs. Now we catch the failure and re-add (single-expiration) jobs to the start of the priority queue.
This is the missing piece to #4846.
This is an incremental change, so that we don't have to touch the happy path. A rewrite would be justified to collapse gathering and self-sends.
There is an optimisation realised in
@prune, the same could be done ingatherExpired(n.post)with a slight restructuring of the conditions: