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

Fix task may appear to be triggered more than 1 second late #27

Merged
merged 3 commits into from
Aug 27, 2022

Conversation

progheal
Copy link
Contributor

The calculation of next trigger time do not discard fractional seconds. Depending on the exact timing of ticking, the task may appear to be triggered more than 1 second late, despite properly ticked every second.

Example: Suppose a task is scheduled to trigger at 00 second, but the calculated next trigger time is at 00.5 second, while tick() is happening at every .35 second. At 00.35 second a tick happened, but because 00.35 < 00.5 the task is not triggered. It will be triggered in the next tick that's at 01.35 second, which appeared to be late for 1.35 second (despite get_delay() will return 1.35 - 0.5 = 0.85 second in this case).

The source of the fractional seconds is from the from argument, where now(), a clock that usually have sub-second precision, is passed. If during the calculation, date_changed is never true, the variable curr will only be modified through += and -= in the second half of the function, where the increments and/or decrements are all multiple of whole seconds.

This fix calculates the fractional second by modulo 1 second, then subtracted that from the calculated time. Since the trigger time is now scheduled at exact second, whenever tick happened within that second the task will be triggered. Doing floor by std::duration_cast<> is not good because it changes the duration type (because of different ratio), and therefore the the time_point type (because of different duration).

@PerMalmberg
Copy link
Owner

PerMalmberg commented Jul 12, 2022

Hello @progheal

This fix seems correct, thank you for tracking it down and creating this PR.

At the moment I'm fully occupied with my summer vacation away from coding so I won't setup my environment to run the unit tests to verify it haven't broken anything. As such I will leave this PR open until that can be arranged.

What we really need is GH Actions to run these. Should you have the time to set that up I'll happily merge that too.

Edit: Also, please add a comment to the new code so it is clear why it is needed. I know it can be found via Git, but I think a comment is worthwhile here.

@progheal
Copy link
Contributor Author

Comment added. Have a nice vacation!

About setting up GH Action, unfortunately I don't have much free time either. I've skimmed through the documents, but because I have never used them before, I don't really know how many time it will take. Sorry about that.

@PerMalmberg
Copy link
Owner

Tests passed :)

Thanks for your contribution.

@PerMalmberg PerMalmberg merged commit 0dd9df4 into PerMalmberg:master Aug 27, 2022
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 this pull request may close these issues.

2 participants