Skip to content

Ensure that all async generators are explicitly closed #1019

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

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

agronholm
Copy link
Contributor

Summary

This change avoids warnings on Trio by explicitly closing all async generators rather than relying on the garbage collector to do so, as this may cause unpredictable behavior due to different GC implementations.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@agronholm
Copy link
Contributor Author

There are no new tests but I've removed the xfail markers for the existing cancellation tests.

@agronholm
Copy link
Contributor Author

agronholm commented Jun 18, 2025

Would it be okay to upgrade Mypy? Later versions are fine with the single type parameter to AsyncGenerator.

@tomchristie
Copy link
Member

Would it be okay to upgrade Mypy?

Yep.

You're welcome to take the shortcut and update that here, or else issue a separate PR dealing just with that.

@agronholm
Copy link
Contributor Author

What's the policy on Python support btw? Do you want to keep supporting the EOL'd 3.8?

@tomchristie
Copy link
Member

Okay with dropping support for EOL'd Pythons.

@agronholm
Copy link
Contributor Author

agronholm commented Jun 23, 2025

Okay with dropping support for EOL'd Pythons.

Done. I also updated the setup-python action and added Python 3.13 to the CI. This currently makes its CI job take longer than the other Python versions, due to the facts that 1) the workflow doesn't use a pip cache, 2) it installs a lot of heavy dependencies which have nothing to do with running the test suite and 3) it pins matplotlib to an old version that does not have Python 3.13 wheels. I can fix any of the above issues, just let me know what you'd like me to do.

@agronholm
Copy link
Contributor Author

Note that I had to add two # pragma: no cover markers as otherwise coverage would drop due to diverging code paths based on the Python version. Let me know if you'd like me to switch the CI to use coverage combining instead.

@agronholm
Copy link
Contributor Author

I found a couple more spots where async iterables that may be generators aren't closed.

@agronholm agronholm requested a review from Kludex June 26, 2025 15:32
@agronholm
Copy link
Contributor Author

@Kludex I replaced the awkward AsyncExitStack mess with a much cleaner solution which I also used in my httpx PR (encode/httpx#3593). Sorry for the extra churn!

@Kludex Kludex mentioned this pull request Jun 27, 2025
3 tasks
requirements.txt Outdated
@@ -15,7 +15,7 @@ twine==6.1.0
# Tests & Linting
coverage[toml]==7.5.4
ruff==0.5.0
mypy==1.16.1
mypy==1.14.1
Copy link

Choose a reason for hiding this comment

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

Maybe un-downgrade mypy, since Python 3.8 support was dropped.

@Kludex
Copy link
Member

Kludex commented Jul 17, 2025

@tomchristie This PR and the one in httpx are pre-requisites to a PR @agronholm is working in here: modelcontextprotocol/python-sdk#946 (I'm helping on that project).

I know I have merge rights here, but I don't feel comfortable merging here. 👀

Would you mind checking if something is needed on those PRs to move forward? 🙏

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.

4 participants