AuthorizedSession's _auth_request sub-session is never cleaned #658

Environment details

• OS: macOS
• Python version: 3.9
• google-auth version: 1.24.0

Steps to reproduce (kinda)

Py.test 6.2 enables hard errors on unhandled thread exceptions and resource errors such as leaking SSL sockets/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)>

warnings, and digging deep into google.auth, I found this problematic segment.

As you may be aware, requests.Session() objects should be .close()d (or used as context managers) so the underlying connection pool gets shut down.

The above-linked code path creates a separate requests.Session() and a google.auth.transport.requests.Request object using it when no explicit Request object is passed in (and this seems to be the default for a trivial use of e.g. google-cloud-storage).

When that happens, the secondary session (only used for refreshing credentials) is never cleaned up even if the owning session might be.

Aside on spurious logging

As an aside, this same code path causes a

urllib3.util.retry[85743] DEBUG: Converted retries value: 3 -> Retry(total=3, connect=None, read=None, redirect=None, status=None)

message to be logged since the sub-session's HTTP adapter is initialized with max_retries=3; this is indeed what the requests manual says, but changing that to max_retries=Retry(3, redirect=True) (where Retry comes from urllib3) would fix that.

Remedy

I think AuthorizedSession should remember whether it initialized this subsession, and if it did, have it clean it up on close(); something like

def close(self):
    if self._auth_request_session:
        self._auth_request_session.close()
    super().close()

might do since close() is already automatically called by requests.Session.__exit__(), so careful users of AuthorizedSession will benefit from this immediately.

I'm willing to formalize this into a PR if the plan sounds good.

Workaround

I'm currently making sure to close those pools with a hack like this.

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

    def __exit__(self, *args):
        try:
            # Attempt closing the implicit sub-session
            # https://github.com/googleapis/google-auth-library-python/issues/658
            self._http._auth_request.session.close()
        except Exception:
            pass
        self._http.close()