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

Check active even if Condvar times out in PeriodicWorker #61

Merged
merged 1 commit into from
Feb 11, 2024

Conversation

SpriteOvO
Copy link
Owner

@SpriteOvO SpriteOvO commented Feb 5, 2024

Fixes #60.

Before this PR, we didn't check the active flag if wait_timeout_while returns due to a timeout. There may be a small probability issue with this, i.e. PeriodicWorker being dropped a short time after the wait_timeout_while timeout has returned will potentially result in an infinite join.

@SpriteOvO SpriteOvO changed the title Allow clippy lint unused_imports for sync exports Check active even if Condvar times out in PeriodicWorker Feb 5, 2024
Copy link
Collaborator

@Lancern Lancern left a comment

Choose a reason for hiding this comment

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

PeriodicWorker being dropped a short time after the wait_timeout_while timeout has returned will potentially result in an infinite join.

The changes LGTM, but is the situation described above really possible?

@SpriteOvO
Copy link
Owner Author

but is the situation described above really possible?

Just a conjectural situation, this is mainly to fix the error reported by rustc in #60, and an extra check at least seems harmless. However I can try to write a minimal reproducible later to verify if this situation is possible.

@SpriteOvO
Copy link
Owner Author

SpriteOvO commented Feb 11, 2024

I was wrong, I can't verify that situation, Condvar::wait_timeout_while always checks for the condition: F, regardless of whether it's woken up or not. It doesn't seem to make much difference whether the lock guard is held or not here. Let's merge it just for suppressing the rustc lint error.

@SpriteOvO SpriteOvO merged commit 40e5c18 into main-dev Feb 11, 2024
64 checks passed
@SpriteOvO SpriteOvO deleted the fix-periodic-worker-nightly-err branch February 11, 2024 08:46
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