bugfix: check that timer servicing worked #4846
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
ggreif
commented
Jan 14, 2025
that sets the global timer for ASAP retry
crusso
reviewed
Jan 15, 2025
crusso
reviewed
Jan 15, 2025
src/ir_passes/await.ml
Outdated
| (* if self-call queue full: expire global timer soon and retry *) | ||
| and t_timer_throw context exp = | ||
| t_on_throw context exp | ||
| (blockE [expD (primE |
There was a problem hiding this comment.
Are we sure there aren't other reasons for the error than queue full? If so, it might make make sense to test the error code of the error and only set the global timer when appropriate...
There was a problem hiding this comment.
Checking the Wasm, the only possibility is send failure.
crusso
reviewed
Jan 15, 2025
A PR on top of this one and with the logic to insert the failed expirations into the priority queue's head (in order). |
review feedback
|
Download the artifacts for this pull request: |
berestovskyy
previously approved these changes
Jan 16, 2025
There was a problem hiding this comment.
Seems it fixes the issue, I can't reproduce it anymore on my branch https://github.com/dfinity/ic/commits/andriy/motoko-timers-repro/
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`.
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 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
asynctimer servicing routine cannot be run. The fix is simple, check if the self-call succeeded (causes athrowalready), and if not, set a very near global timer to retry ASAP (in the top-levelcatch).TODO:
catchsend errors for user workers (and mitigate) — see Mitigate timer job submission failures #4852