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

Dont perform implicit close/warning on __del__ #2026

merged 4 commits into from Jan 13, 2022


Copy link

We currently attempt to perform a bit of hand-holding for cases where a client instance has not been closed at the point it is deleted. The reasoning here was to try to give users a bit more of a guard against unintentional resource leaks.

Our behaviour here was...

  • For sync clients, automatically .close() clients on __del__, if they're still open.
  • For async clients, issue a warning on __del__, if they're still open.

As it turns out, this just isn't reliable. We really can't do this properly, because if __del__ is called at atexit then there's all sorted of bit of stdlib that may not be available. We can't reliably call into warnings.warn, and it turns out we can't even reliably access the properties on an intenum class. Geez.

So. Let's just not do this anymore.

If you're leaking client instances (which as it happens, zillions of requests and urllib3 using codebases currently already do.) Then okay, that's just the way it is. I'm not sure what flags you'd need to enable in python to have the system warn you about when that's the case, but we should rely on that level of warning, rather than attempting to add any ourselves.

Better that we stick with the current status-quo, than attempt to do better and actually just end up with user-hostile exceptions or undefined behaviours.

@tomchristie tomchristie added the bug Something isn't working label Jan 12, 2022
@tomchristie tomchristie merged commit 7f0d43d into master Jan 13, 2022
5 checks passed
@tomchristie tomchristie deleted the dont-perform-implicit-close-on-del branch January 13, 2022 10:37
@tomchristie tomchristie mentioned this pull request Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
bug Something isn't working
None yet

Successfully merging this pull request may close these issues.

httpx\", line 1991, in __del__ AttributeError: 'NoneType' object has no attribute 'OPENED'
1 participant