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

Keep a reference to tasks #67

Closed
wants to merge 3 commits into from

Conversation

davidbrochart
Copy link
Contributor

Tasks may disappear while executing because of garbage collection. See the documentation:

image

@coveralls
Copy link

coveralls commented Mar 16, 2023

Coverage Status

Coverage: 97.256% (+0.009%) from 97.247% when pulling d7b506c on davidbrochart:tasks into ee1f1c0 on asphalt-framework:master.

@agronholm
Copy link
Member

I think this needs a new test to ensure that tasks don't get dropped.

@davidbrochart
Copy link
Contributor Author

I think this test is enough, since the coroutine callback doesn't hold any reference to an outside object, and only has a side effect. But it is very unlikely to fail even without this PR, because it will probably not be garbage collected anyway.

@agronholm
Copy link
Member

The basic requirement for any fix PR is to demonstrate that the change does in fact fix a problem. CPython should be very consistent in this manner, meaning that once the references to an object drop to zero, it's immediately gc'd. If you can't demonstrate this behavior, then what does this actually fix?

@davidbrochart
Copy link
Contributor Author

Right.
I've been trying to trigger the bug without this PR, using gc.collect(), but I cannot. I think the task cannot be garbage-collected because of its reference to the event.
Should we close this PR then?

@agronholm
Copy link
Member

I think so. If the event system was dropping tasks, I, you or someone else would probably have noticed it before.

@davidbrochart davidbrochart deleted the tasks branch March 25, 2023 12:33
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.

None yet

3 participants