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

bleak: fix leaking of ensure_future() #1259

Merged
merged 1 commit into from
Mar 20, 2023
Merged

Conversation

dlech
Copy link
Collaborator

@dlech dlech commented Mar 18, 2023

As noted in the Python docs, a reference to the return value of asyncio.ensure_future() must be held in order to prevent it from being garbage collected before the task completes.

This applies the recommended fix from the docs of holding the reference in a global set and then discarding the reference when the task completes.

Also change asyncio.ensure_future() to asyncio.create_task() while we are touching this since the minimum supported Python version is now 3.7.

Fixes: #1258

As noted in the Python docs, a reference to the return value of
`asyncio.ensure_future()` must be held in order to prevent it from
being garbage collected before the task completes.

This applies the recommended fix from the docs of holding the reference
in a global set and then discarding the reference when the task
completes.

Also change `asyncio.ensure_future()` to `asyncio.create_task()` while
we are touching this since the minimum supported Python version is now
3.7.

Fixes: #1258
@dlech
Copy link
Collaborator Author

dlech commented Mar 18, 2023

cc: @bdraco

@bdraco
Copy link
Contributor

bdraco commented Mar 18, 2023

It might be safer to bind it to the scanner or client object as self._background_tasks so if the task never finishes for some reason and the scanner/client is no longer used and goes out of scope the task and the scanner/client its holding a reference to will get garbaged collected eventually.

That will only work if its ok for the task to get GCed when the scanner/client goes out of scope (I think it is?)

If we are sure the task will always eventually finish and release its references to the object than it probably doesn't matter.

@dlech
Copy link
Collaborator Author

dlech commented Mar 18, 2023

That will only work if its ok for the task to get GCed when the scanner/client goes out of scope (I think it is?)

I don't think we ever want to have a task get GCed before completing, otherwise it could just stop in the middle of a coroutine without ever cancelling is which could lead to undefined/unexpected behavior.

This type of fire-and-forget programming is a bit of an anti-pattern IMHO, so if people want to use it, I'm inclined to let them work out the details of how to cancel the task to avoid leaks if needed.

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.

The disconnect monitor task can get garbage collected before it finishes
2 participants