-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(rab): run async background boundary refresh on detached session #17441
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
base: main
Are you sure you want to change the base?
Changes from all commits
dab0d4c
fab83f1
3240f44
4312df8
b36e583
d11fd64
d203e75
9eaed0b
31404d7
4a79bdd
74fcfac
00ffcdb
8153425
5e2fbc2
cef4bbc
6d0fd3a
ebaa1d5
cae43cf
05ffb8a
2d17e65
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -142,3 +142,13 @@ async def close(self) -> None: | |
| Close the underlying session. | ||
| """ | ||
| raise NotImplementedError("close must be implemented.") | ||
|
|
||
| def _clone(self) -> "Request": | ||
| """Creates a copy of this request adapter. | ||
|
|
||
| The base implementation returns `self` (an identical shared instance). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I still think a name other than clone should be considered. Gemini suggests _isolate() or _branch(). But this doesn't matter too much if it's internal |
||
| Transport adapters that maintain internal connection pools or stateful | ||
| sessions must override this method to return an independent, detached | ||
| adapter instance. | ||
| """ | ||
| return self | ||
|
nbayati marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,7 @@ | |
| else: | ||
| try: | ||
| from aiohttp import ClientTimeout | ||
| except (ImportError, AttributeError): | ||
| except (ImportError, AttributeError): # pragma: NO COVER | ||
| ClientTimeout = None | ||
|
|
||
| _LOGGER = logging.getLogger(__name__) | ||
|
|
@@ -203,3 +203,83 @@ async def close(self) -> None: | |
| if not self._closed and self._session: | ||
| await self._session.close() | ||
| self._closed = True | ||
|
|
||
| def _clone(self) -> "Request": | ||
| """Creates an independent copy of this request adapter. | ||
|
|
||
| Clones the connection settings, trace configurations, and session defaults | ||
| (headers, cookies, basic auth, and timeouts). | ||
|
|
||
| Only standard `aiohttp.TCPConnector` and `aiohttp.UnixConnector` connectors | ||
| are supported. The DNS resolver is not copied to avoid closing shared resolver | ||
| resources. | ||
|
|
||
| Returns: | ||
| google.auth.aio.transport.aiohttp.Request: A new request adapter. | ||
|
|
||
| Raises: | ||
| google.auth.exceptions.TransportError: If the transport is closed, or if the | ||
| session uses an unsupported connector. | ||
| """ | ||
| if self._closed: | ||
| raise exceptions.TransportError("Cannot clone a closed transport.") | ||
|
|
||
| if not self._session: | ||
| new_session = aiohttp.ClientSession( | ||
| auto_decompress=False, | ||
| trust_env=True, | ||
| ) | ||
| return Request(session=new_session) | ||
|
|
||
| session_kwargs: dict = { | ||
| "auto_decompress": False, | ||
| "trust_env": getattr(self._session, "_trust_env", True), | ||
| } | ||
|
|
||
| # Copy underlying connection pool settings (SSL context, IP bindings, limits). | ||
| orig_connector = getattr(self._session, "_connector", None) | ||
| if orig_connector and not orig_connector.closed: | ||
| if isinstance(orig_connector, aiohttp.TCPConnector): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The This silently drops the proxy/custom configuration and routes traffic directly over the public internet, which will fail or violate security constraints in enterprise/isolated cloud environments. We should either explicitly support proxy preservation or raise a clear transport exception if an unsupported custom connector is detected.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a really good point. We can't really support proxy preservation because third-party aiohttp connectors have arbitrary, unknown constructor signatures (meaning we have no way to instantiate a fresh detached copy of them dynamically), and simply shallow-copying the existing connector is unsafe due to shared socket pools. This leaves us two options: fallback to re-using the customer transport and hope that we don't encounter the bug this PR is trying to fix, or raise the exception as you suggested and accept this as a limitation of RAB. I've decided not to fallback to re-using the customer's transport if we can't clone it, because it's not just that the RAB call would fail, but also there's another risk: if the foreground task closes the session while the background worker is actively reading from it, the forceful socket truncation mid-flight can leave complex corporate proxy connections in a hung or corrupted state, which means that the affects won't be limited to our RAB calls. So I've added the else: raise exceptions.TransportError(...) block, as raising the error here is the safest path. The exception will trigger the 15-minute cooldown and allow the user's main request to proceed safely. I thought about disabling RAB permanently if we can't clone the transport (thinking what's the point of entering cooldown if we're going to keep trying to clone it and fail), but decided against it. I realized that because credentials objects are frequently instantiated globally and shared across multiple different clients and API surfaces, there's a chance that the next call would be executed over entirely different transports, making the RAB call possible. |
||
| # We explicitly do not copy the resolver. The connector | ||
| # owns the resolver, and closing the cloned session would | ||
| # close the shared resolver, breaking the original session. | ||
| session_kwargs["connector"] = aiohttp.TCPConnector( | ||
| ssl=getattr(orig_connector, "_ssl", None), # type: ignore | ||
| limit=getattr(orig_connector, "_limit", 100), | ||
| limit_per_host=getattr(orig_connector, "_limit_per_host", 0), | ||
| force_close=getattr(orig_connector, "_force_close", False), | ||
| local_addr=getattr(orig_connector, "_local_addr", None), | ||
| ) | ||
| elif getattr(aiohttp, "UnixConnector", None) and isinstance( | ||
| orig_connector, getattr(aiohttp, "UnixConnector") | ||
| ): | ||
| path = getattr(orig_connector, "_path", None) | ||
| if path: | ||
| session_kwargs["connector"] = aiohttp.UnixConnector( | ||
| path=path, | ||
| limit=getattr(orig_connector, "_limit", 100), | ||
| force_close=getattr(orig_connector, "_force_close", False), | ||
| ) | ||
| else: | ||
| raise exceptions.TransportError( | ||
| f"Unsupported connector type for cloning: {type(orig_connector)}" | ||
| ) | ||
|
|
||
| # Preserve distributed tracing configurations. | ||
| trace_configs = getattr(self._session, "_trace_configs", None) | ||
| if trace_configs: | ||
| session_kwargs["trace_configs"] = list(trace_configs) | ||
|
|
||
| # Copy session-level defaults (headers, cookies, auth, timeout). | ||
| for attr_name, kwarg_name in [ | ||
| ("_default_headers", "headers"), | ||
| ("_cookie_jar", "cookie_jar"), | ||
| ("_default_auth", "auth"), | ||
| ("_timeout", "timeout"), | ||
| ("_json_serialize", "json_serialize"), | ||
| ]: | ||
| val = getattr(self._session, attr_name, None) | ||
| if val is not None: | ||
| session_kwargs[kwarg_name] = val | ||
|
|
||
| return Request(session=aiohttp.ClientSession(**session_kwargs)) # type: ignore | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -149,6 +149,7 @@ def __init__(self, session=None): | |
| "Client sessions with auto_decompress=True are not supported." | ||
| ) | ||
| self.session = session | ||
| self._closed = False | ||
|
|
||
| async def __call__( | ||
| self, | ||
|
|
@@ -184,6 +185,9 @@ async def __call__( | |
| """ | ||
|
|
||
| try: | ||
| if getattr(self, "_closed", False): | ||
| raise exceptions.TransportError("session is closed.") | ||
|
|
||
| if self.session is None: # pragma: NO COVER | ||
| self.session = aiohttp.ClientSession( | ||
| auto_decompress=False | ||
|
|
@@ -203,6 +207,92 @@ async def __call__( | |
| new_exc = exceptions.TransportError(caught_exc) | ||
| raise new_exc from caught_exc | ||
|
|
||
| def _clone(self): | ||
| """Creates an independent copy of this request adapter. | ||
|
|
||
| Clones the connection settings, trace configurations, and session defaults | ||
| (headers, cookies, basic auth, and timeouts). | ||
|
|
||
| Only standard `aiohttp.TCPConnector` and `aiohttp.UnixConnector` connectors | ||
| are supported. The DNS resolver is not copied to avoid closing shared resolver | ||
| resources. | ||
|
|
||
| Returns: | ||
| google.auth.transport._aiohttp_requests.Request: A new request adapter. | ||
|
|
||
| Raises: | ||
| google.auth.exceptions.TransportError: If the transport is closed, or if the | ||
| session uses an unsupported connector. | ||
| """ | ||
| if getattr(self, "_closed", False): | ||
| raise exceptions.TransportError("Cannot clone a closed transport.") | ||
|
|
||
| if not self.session: | ||
| new_session = aiohttp.ClientSession( | ||
| auto_decompress=False, | ||
| trust_env=True, | ||
| ) | ||
| return Request(session=new_session) | ||
|
|
||
| session_kwargs: dict = { | ||
| "auto_decompress": False, | ||
| "trust_env": getattr(self.session, "_trust_env", True), | ||
| } | ||
|
|
||
| # Copy underlying connection pool settings (SSL context, IP bindings, limits). | ||
| orig_connector = getattr(self.session, "_connector", None) | ||
| if orig_connector and not getattr(orig_connector, "closed", True): | ||
| if isinstance(orig_connector, aiohttp.TCPConnector): | ||
| # We explicitly do not copy the resolver. The connector | ||
| # owns the resolver, and closing the cloned session would | ||
| # close the shared resolver, breaking the original session. | ||
| session_kwargs["connector"] = aiohttp.TCPConnector( | ||
| ssl=getattr(orig_connector, "_ssl", None), # type: ignore | ||
| limit=getattr(orig_connector, "_limit", 100), | ||
| limit_per_host=getattr(orig_connector, "_limit_per_host", 0), | ||
| force_close=getattr(orig_connector, "_force_close", False), | ||
| local_addr=getattr(orig_connector, "_local_addr", None), | ||
| ) | ||
| elif getattr(aiohttp, "UnixConnector", None) and isinstance( | ||
| orig_connector, getattr(aiohttp, "UnixConnector") | ||
| ): | ||
| path = getattr(orig_connector, "_path", None) | ||
| if path: | ||
| session_kwargs["connector"] = aiohttp.UnixConnector( | ||
| path=path, | ||
| limit=getattr(orig_connector, "_limit", 100), | ||
| force_close=getattr(orig_connector, "_force_close", False), | ||
| ) | ||
| else: | ||
| raise exceptions.TransportError( | ||
| f"Unsupported connector type for cloning: {type(orig_connector)}" | ||
| ) | ||
|
|
||
| # Preserve distributed tracing configurations. | ||
| trace_configs = getattr(self.session, "_trace_configs", None) | ||
| if trace_configs: | ||
| session_kwargs["trace_configs"] = list(trace_configs) | ||
|
|
||
| # Copy session-level defaults (headers, cookies, auth, timeout). | ||
| for attr_name, kwarg_name in [ | ||
| ("_default_headers", "headers"), | ||
| ("_cookie_jar", "cookie_jar"), | ||
| ("_default_auth", "auth"), | ||
| ("_timeout", "timeout"), | ||
| ("_json_serialize", "json_serialize"), | ||
| ]: | ||
| val = getattr(self.session, attr_name, None) | ||
| if val is not None: | ||
| session_kwargs[kwarg_name] = val | ||
|
|
||
| return Request(session=aiohttp.ClientSession(**session_kwargs)) # type: ignore | ||
|
|
||
| async def close(self): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the legacy _aiohttp_requests transport, calling call on an already-closed instance propagates a raw aiohttp RuntimeError rather than a wrapped google.auth.exceptions.TransportError because call does not inspect self._closed (unlike the modern successor in aio/transport/aiohttp.py). While this legacy adapter is deprecated/internal and closed reuse is non-standard, checking self._closed in call and raising TransportError directly would align exception behaviors.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Updated |
||
| """Cleanly release the underlying aiohttp ClientSession resources.""" | ||
| if not getattr(self, "_closed", False) and self.session: | ||
| await self.session.close() | ||
| self._closed = True | ||
|
|
||
|
|
||
| class AuthorizedSession(aiohttp.ClientSession): | ||
| """This is an async implementation of the Authorized Session class. We utilize an | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It seems like
_prepare_async_lookup_callableand_close_cloned_requestwould be a good fit for a context manager. That would let us encapsulate these three variables, and enforce automatic closing.Gemini put this together:
But this is just a suggestion that came to mind, I think it's fine to merge as-is too.