-
Notifications
You must be signed in to change notification settings - Fork 16
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
Clients do not clean up Requests sessions #64
Comments
Thanks @akx! CC @crwilcox as a We've talked about a |
FWIW, I'd argue that this is a feature request, rather than a bug (not that I'm opposed to adding it). |
I'd also suggest that the existence of |
FBO use with 'contextlib.closing'. Closes #64.
FBO use with 'contextlib.closing'. Closes #64.
FBO use with 'contextlib.closing'. Closes #64.
FBO use with 'contextlib.closing'. Closes #64.
Environment details
google-cloud-core
version: 1.4.3google-cloud-storage
version: 1.33.0 (since it's related)Steps to reproduce
This is a sibling issue to googleapis/google-auth-library-python#658 and as such the below is similar to that issue.
Py.test 6.2 enables hard errors on unhandled thread exceptions and resource errors such as leaking sockets/SSL sockets/FDs/.... I've spent the last couple hours debugging various
When initiating (e.g.) a
google-cloud-storage
storage client with e.g.the client object is initialized with no explicit
_http
object, and the default code path is run, creating a newrequests.Session()
that will be eventually GC'd but is otherwise notclose()
d, leading to the above warning (and a dropped connection and socket, which may have implications down the line).Since
google.core.client.Client
doesn't implement e.g. the context manager protocol orclose()
, there's no officially sanctioned way to clean up that session; for a storage Client, I subclassed one as a workaround but it would be nice if Clients had an officialclose()
.Resolution suggestion
At the very least, add
close()
toClient
s; by default it should probably only be something like, the semantics being a best-effort cleanup of resources allocated by the client, but not irreversible finalization.
With this,
Client
s could be used withcontextlib.closing()
. Additionally, one could add a minimal context management protocol implementation á laIf you folks think this is a good way to go, I can certainly write the PR :)
cc @busunkim96 (for having triaged the other issue).
The text was updated successfully, but these errors were encountered: