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

Clean up Timer tasks #323

Closed
wants to merge 1 commit into from
Closed

Clean up Timer tasks #323

wants to merge 1 commit into from

Conversation

Marenz
Copy link
Contributor

@Marenz Marenz commented Sep 23, 2024

No description provided.

Signed-off-by: Mathias L. Baumann <[email protected]>
@Marenz Marenz requested a review from a team as a code owner September 23, 2024 11:57
@Marenz Marenz requested a review from shsms September 23, 2024 11:57
@github-actions github-actions bot added the part:utilities Affects the utility receivers (`Timer`, `Event`, `FileWatcher`) label Sep 23, 2024
@Marenz Marenz added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Sep 23, 2024
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

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

I don't think this is necessary, if you are seeing warning in pytest about tasks being pending, is a bug in pytest: pytest-dev/pytest-asyncio#200

Unless I'm missing something, I think as_completed() is being used properly here.

@Marenz
Copy link
Contributor Author

Marenz commented Sep 24, 2024

It might not be necessary, but I believe it actually speeds up runtime if we clean up our tasks afterwards.

@llucax
Copy link
Contributor

llucax commented Sep 24, 2024

Why do you think that? But in any case, if you want to do eager finalization (so not letting the GC pick up your garbage), then probably this is shorter and should do the same:

            completed = asyncio.as_completed(
                [asyncio.sleep(time_to_next_tick / 1_000_000), self._reset_event.wait(),]
            )

            try:
                await next(completed)
            finally:
                del completed

@Marenz
Copy link
Contributor Author

Marenz commented Sep 24, 2024

Well, now I did the actual measurement and it does not seem to help at all XD

@Marenz Marenz closed this Sep 24, 2024
auto-merge was automatically disabled September 24, 2024 11:14

Pull request was closed

@Marenz Marenz deleted the rearm branch September 30, 2024 08:20
@Marenz Marenz restored the rearm branch September 30, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd:skip-release-notes It is not necessary to update release notes for this PR part:utilities Affects the utility receivers (`Timer`, `Event`, `FileWatcher`)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants