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.

Sign up for GitHub

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

Jump to bottom

doc: add notes that timer jobs should be side-effect-free #682

Merged
merged 1 commit into from
Jan 20, 2025
Merged

doc: add notes that timer jobs should be side-effect-free #682

merged 1 commit into from
Jan 20, 2025
+5 −0

Conversation

ggreif
Copy link
Contributor

@ggreif ggreif commented Jan 17, 2025 •
edited
Loading

... other than submitting the self-call, of course

@ggreif ggreif requested a review from a team as a code owner January 17, 2025 14:59
@ggreif ggreif mentioned this pull request Jan 17, 2025
Merged
2 tasks
@ggreif ggreif requested review from crusso and rvanasa January 17, 2025 17:22
@ggreif ggreif self-assigned this Jan 17, 2025
mergify bot pushed a commit to dfinity/motoko that referenced this pull request Jan 17, 2025
@ggreif
bugfix: check that timer servicing worked (#4846)
a9bc214 
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
@ggreif ggreif enabled auto-merge (squash) January 17, 2025 22:49
crusso
crusso reviewed Jan 20, 2025
View reviewed changes
Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually make sense though? The only reason to execute a timer is for a side effect. I'm not sure we need to specify this since we only try to enqueue the job and repeat on failure to enqueue, in which case the failed job has no effect.

@ggreif
Copy link
Contributor Author

ggreif commented Jan 20, 2025 •
edited
Loading

Does this actually make sense though? The only reason to execute a timer is for a side effect. I'm not sure we need to specify this since we only try to enqueue the job and repeat on failure to enqueue, in which case the failed job has no effect.

E.g. somebody could pre-compose the thunk with func() = expirations += 1 and this would have an additional side-effect. Also there could be a trap side effect (see --trap-on-call-error), which would totally kill the global timer.

@crusso
Copy link
Contributor

crusso commented Jan 20, 2025

I don't think there's any way to precomposed without wrapping in another async function but that legacy flag could indeed be problematic.

I'll approve anyway though

1 similar comment
@crusso
Copy link
Contributor

crusso commented Jan 20, 2025

I don't think there's any way to precomposed without wrapping in another async function but that legacy flag could indeed be problematic.

I'll approve anyway though

@crusso crusso self-requested a review January 20, 2025 23:13
@ggreif ggreif merged commit 77d7447 into master Jan 20, 2025
9 checks passed
@ggreif ggreif deleted the gabor/timer-doc branch January 20, 2025 23:13
@ggreif ggreif added the documentation Improvements or additions to documentation label Jan 21, 2025
@ggreif ggreif mentioned this pull request Jan 21, 2025
Merged
ggreif added a commit that referenced this pull request Jan 21, 2025
@ggreif
doc: add notes to Timer.mo (#683)
0f9c22d 
This brings the notes in line with what the Motoko `docs/md/Timer.md`
is.

Basically also reverts #682.

Plus a few tweaks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crusso crusso crusso approved these changes

@rvanasa rvanasa Awaiting requested review from rvanasa

Assignees

@ggreif ggreif

Labels
documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ggreif @crusso