Clients do not clean up Requests sessions

[akx]

Environment details

• OS type and version: macOS
• Python version: 3.9
• google-cloud-core version: 1.4.3
• google-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

ResourceWarning: unclosed <ssl.SSLSocket fd=12, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('192.168.1.23', 60522), raddr=('216.58.207.202', 443)>

When initiating (e.g.) a google-cloud-storage storage client with e.g.

        creds = self.get_client_credentials()
        return storage.Client(project=creds.project_id, credentials=creds)

the client object is initialized with no explicit _http object, and the default code path is run, creating a new requests.Session() that will be eventually GC'd but is otherwise not close()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 or close(), 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 official close().

        class CloseableStorageClient(storage.Client):
            def __enter__(self):
                return self

            def __exit__(self, *args):
                self._http.close()

Resolution suggestion

At the very least, add close() to Clients; by default it should probably only be something like

def close(self):
    if self._http_internal:
        self._http_internal.close()

, the semantics being a best-effort cleanup of resources allocated by the client, but not irreversible finalization.

With this, Clients could be used with contextlib.closing(). Additionally, one could add a minimal context management protocol implementation á la

            def __enter__(self):
                return self

            def __exit__(self, *args):
                self.close()

If you folks think this is a good way to go, I can certainly write the PR :)

cc @busunkim96 (for having triaged the other issue).

[busunkim96]

Thanks @akx!

CC @crwilcox as a google-cloud-storage owner.

We've talked about a close() method for the gapic clients. Ideally the behavior will be similar in both the generated and handwritten clients. googleapis/gapic-generator-python#575

[crwilcox]

Cc @frankyn @andrewsg

[tseaver]

FWIW, I'd argue that this is a feature request, rather than a bug (not that I'm opposed to adding it).

[tseaver]

I'd also suggest that the existence of contextlib.closing makes it unnecessary to add our own __enter__ / __exit__ methods.