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 timer reset() while it is being waited on #241

Merged
merged 3 commits into from
Nov 22, 2023

Conversation

llucax
Copy link
Contributor

@llucax llucax commented Nov 21, 2023

If the timer was reset while it was being waited on, the next tick time was not recalculated and the timer would wait for the original time instead of the new one.

This means a timeout could fire even if there was actually no timeout (because the timer was reset).

@llucax llucax requested a review from a team as a code owner November 21, 2023 11:52
@llucax llucax self-assigned this Nov 21, 2023
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:core Affects the core types (`Sender`, `Receiver`, exceptions, etc.) labels Nov 21, 2023
@llucax llucax added this to the v1.0.0 milestone Nov 21, 2023
@llucax llucax added the type:bug Something isn't working label Nov 21, 2023
@llucax
Copy link
Contributor Author

llucax commented Nov 21, 2023

Based on #240.

The test is flaky, I will update it to remove the flakiness.

@llucax
Copy link
Contributor Author

llucax commented Nov 21, 2023

Updated, hopefully this time it passes all the tests.

@llucax
Copy link
Contributor Author

llucax commented Nov 21, 2023

#240 merged, rebased.

@llucax
Copy link
Contributor Author

llucax commented Nov 21, 2023

Enabled auto-merge, please don't approve if you have suggestions.

Copy link
Contributor

@daniel-zullo-frequenz daniel-zullo-frequenz left a comment

Choose a reason for hiding this comment

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

I have a couple of suggestions to check for. LGTM otherwise

src/frequenz/channels/timer.py Outdated Show resolved Hide resolved
tests/test_timer_integration.py Show resolved Hide resolved
Copy link
Contributor

@daniel-zullo-frequenz daniel-zullo-frequenz left a comment

Choose a reason for hiding this comment

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

Just have a question about the missing copyright/license header in the test

@@ -0,0 +1,34 @@
# License: MIT
# Copyright © 2022 Frequenz Energy-as-a-Service GmbH
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Copyright © 2022 Frequenz Energy-as-a-Service GmbH
# Copyright © 2023 Frequenz Energy-as-a-Service GmbH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

This test needs to be an integration test because async-solipsism would
make any sleep return immediately, so we wouldn't have a way to do the
reset while the timer is being waited on.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
If the timer was reset while it was being waited on, the next tick time
was not recalculated and the timer would wait for the original time
instead of the new one.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
@llucax llucax added this pull request to the merge queue Nov 22, 2023
Merged via the queue into frequenz-floss:v1.x.x with commit 45b68bf Nov 22, 2023
14 checks passed
@llucax llucax deleted the fix-timer-reset branch November 22, 2023 12:44
@llucax llucax modified the milestones: v1.0.0, v1.0.0-rc.1 Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:core Affects the core types (`Sender`, `Receiver`, exceptions, etc.) part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests type:bug Something isn't working
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants