Skip to content

Fix potential memory leak with persistent task groups#140

Closed
achimnol wants to merge 1 commit intogeldata:masterfrom
achimnol:master
Closed

Fix potential memory leak with persistent task groups#140
achimnol wants to merge 1 commit intogeldata:masterfrom
achimnol:master

Conversation

@achimnol
Copy link
Copy Markdown

@achimnol achimnol commented Dec 16, 2020

I just discovered a potential memory leak issue because the task group internally stores references of all spawned tasks and never clears them in _tasks set.
This should not be a problem if the users of TaskGroup only uses it for short-lived asyncio tasks, but if they use it for a whole lifecycle of long-lived objects or the entire application, it would be a source of memory leak.

Changing _tasks to weakref.WeakSet automatically clears (weak) references to completed tasks, and this does not affect the behavior of the current TaskGroup implementation.

Please have a look in my personal fork's self-PR at achimnol/aiotools#21.
I've skipped adding test cases because there seems no place for TaskGroup test cases.

* Check out the added test cases in my derivation: achimnol/aiotools#21
@gel-data-cla
Copy link
Copy Markdown

gel-data-cla bot commented Dec 16, 2020

All committers signed the Contributor License Agreement.
CLA signed

@1st1
Copy link
Copy Markdown
Member

1st1 commented Dec 17, 2020

I just discovered a potential memory leak issue because the task group internally stores references of all spawned tasks and never clears them in _tasks set.

I'm not sure if it's an actual memory leak. Tasks don't store references to the TaskGroup that spawned them so there's no cyclic dependency here. And once a TaskGroup is GCed, all of its attributes will be cleared, including the set of tasks. So do you have any evidence that the current design causes problems?

I've skipped adding test cases because there seems no place for TaskGroup test cases.

Can you make this PR against the edgedb/edgedb repo? There are tests there (in tests/common/test_taskgroups.py)

@achimnol
Copy link
Copy Markdown
Author

achimnol commented Dec 18, 2020

As I said, when TaskGroup objects are short-lived, it's okay.
But when TaskGroup objects are kept alive for a long time, e.g., for the entire lifecycle of a server daemon, with constantly repeatedly spawned tasks on them, then the references to asyncio tasks will be accumulated. This use case is to make the reclamation of such fire-and-forget tasks to be more explicit (i.e., control the order & timing of cancellation between different task groups), instead of relying on the implicit cancellation of tasks upon event loop shutdown.
I'll recreate the PR there with test cases in this weekend.

@achimnol
Copy link
Copy Markdown
Author

BTW, how is the progress about incorporating TaskGroup APIs into the standard asyncio library?
I've searched it several times and it seems to be stuck at design of MultiError. Is there further progress and/or discussions?

@1st1
Copy link
Copy Markdown
Member

1st1 commented Dec 18, 2020

But when TaskGroup objects are kept alive for a long time, e.g., for the entire lifecycle of a server daemon, with constantly repeatedly spawned tasks on them, then the references to asyncio tasks will be accumulated. This use case is to make the reclamation of such fire-and-forget tasks to be more explicit (i.e., control the order & timing of cancellation between different task groups), instead of relying on the implicit cancellation of tasks upon event loop shutdown.

Right, makes sense. I'm torn between making it a weakset vs keeping it a regular set but just having tasks remove themselves from it when they're done. Thoughts?

BTW, how is the progress about incorporating TaskGroup APIs into the standard asyncio library?

There's a small WG with Guido, Nathaniel, a few other people and I working on ExceptionGroups. That's the fundamental blocker for having task groups in asyncio. No eta yet, but at least we started working on them. :/

@achimnol
Copy link
Copy Markdown
Author

Right, makes sense. I'm torn between making it a weakset vs keeping it a regular set but just having tasks remove themselves from it when they're done. Thoughts?

If WeakSet does not introduce side-effects, I think, why not using an automated clean-up mechanism?
Could you tell me if you have extra concerns on using WeakSet?

@1st1
Copy link
Copy Markdown
Member

1st1 commented Aug 12, 2021

I've merged this PR to the edgedb repo where TaskGroups are used at runtime. In case of edgedb-python they're only used in tests, so we don't really care about this specific issue.

@1st1 1st1 closed this Aug 12, 2021
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