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

Feature Request: allow clients to act as context manager and add close() method to cleanup resources #575

Closed
tswast opened this issue Aug 18, 2020 · 5 comments · Fixed by #987
Labels
generator Bugs, features, and so forth pertaining to the generated client surface priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@tswast
Copy link

tswast commented Aug 18, 2020

Since a transport object may be passed into a client constructor, the client does not close that transport in when the client object is deleted. This results in leaking socket resources in programs that create client objects dynamically.

Proposed design:

Add the following two ways to explicitly clean-up sockets owned by the client: (1, preferred) allow clients to act as a context manager and (2) a close() method.

  1. Context manager

    Allow all client objects to be used as a context manager. Call close() on the client in the exit method to clean up sockets owned by the client.

    In code samples with short-lived clients, this is the cleanup method we should recommend.

Example:

from google.cloud import bigquery_datatransfer_v1

with bigquery_datatransfer_v1.DataTransferServiceClient() as client:
    print(client.get_transfer_run('some run id').state)
  1. close() method

    Add a close() method to all client objects. When called, clean up sockets owned by the client, including the transport channel.

    Example:

from google.cloud import bigquery_datatransfer_v1

client = bigquery_datatransfer_v1.DataTransferServiceClient()
print(client.get_transfer_run('some run id').state)
client.close()  # clean up any open sockets

References:

@tswast
Copy link
Author

tswast commented Aug 18, 2020

Note: this issue is even more important in the microgenerator, as the transport property now appears to be private.

@software-dov
Copy link
Contributor

This is made slightly more complicated because of ownership. Because it's an injected parameter, we can't be sure that the client has the sole reference to the transport; we may wind up closing a transport that other clients are still using. This kind of a corner case, though, and it can probably be ameliorated with documentation and warnings to USE AT YOUR OWN RISK.
I agree, in principle, that having both context management and an explicit close method` are the right choice, but I'd like to spend a little more time mulling details.

@software-dov software-dov added generator Bugs, features, and so forth pertaining to the generated client surface priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Aug 18, 2020
busunkim96 added a commit to googleapis/google-api-python-client that referenced this issue Sep 23, 2020
Fixes #618 🦕

httplib2 leaves sockets open by default. This can lead to situations where programs run out of available sockets. httplib2 added a method last year to clean up connections. https://github.com/httplib2/httplib2/blob/9bf300cdc372938f4237150d5b9b615879eb51a1/python3/httplib2/__init__.py#L1498-L1506

This PR adds two ways to close http connections. The interface is intentionally similar to the proposed design for GAPIC clients. googleapis/gapic-generator-python#575
@tswast
Copy link
Author

tswast commented Sep 29, 2020

To cross-link a bit, this / change in transport is affecting the BigQuery client library. It's a case where the BigQuery client library has ownership of a BigQuery Storage client library that it creates, so we must close it when we're done to avoid leaking system resources.

We're currently working around this issue in googleapis/python-bigquery#278 by calling

self._bqstorage_client._transport.grpc_channel.close()

when done with the BigQuery Storage client.

@software-dov
Copy link
Contributor

Do we need to bump the priority of this feature?

@tseaver
Copy link
Contributor

tseaver commented Jan 13, 2021

Can we just add close, and then document using contextlib.closing as the way to do the cleanup automatically, including a note on the problematic passed-in-transport case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generator Bugs, features, and so forth pertaining to the generated client surface priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants