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

async AuthorizedSession refresh should support async credential refresh method #901

Open
thehesiod opened this issue Oct 26, 2021 · 3 comments
Assignees
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@thehesiod
Copy link

Recently I migrated from my custom async implementation to the beta implementation however I noticed two issues:

  1. _CombinedResponse is missing the reason property and occasionally errors out, my fix:
class _CombinedResponse(_GoogleCombinedResponse):
    @property
    def reason(self):
        return self._response.reason
  1. the credentials refresh does not support an async call (which should be the default if you're using the credentials_async class

fix is making this call async: https://github.com/googleapis/google-auth-library-python/blob/main/google/auth/transport/_aiohttp_requests.py#L371

if sync needs to be supported it should do a check if the method is awaitable/a coroutine or not.

Environment details

  • OS: OSX
  • Python version: 3.8
  • pip version:
  • google-auth version: 2.2.1

Steps to reproduce

  1. auth error during googleapiclient call
@busunkim96 busunkim96 added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Oct 28, 2021
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Jan 26, 2022
@arithmetic1728 arithmetic1728 added priority: p3 Desirable enhancement or fix. May not be included in next release. and removed priority: p2 Moderately-important priority. Fix may not be included in next release. labels Apr 1, 2022
@kkroening
Copy link

This is still an issue, and @thehesiod is right about the fix.

Some more detail...

It looks like the _aio_http_requests.AuthorizedSession definitely expects an async credentials object when it calls before_request:

                await self.credentials.before_request(
                    auth_request, method, url, request_headers
                )

And then it later in the same function, it definitely seems to assume that the credentials are non-async - running the refresh in a background thread but not actually awaiting the coroutine itself:

                    async with self._refresh_lock:
                        await self._loop.run_in_executor(
                            None, self.credentials.refresh, auth_request
                        )

Note: The await here isn't awaiting the refresh call; it's awaiting the background thread without awaiting the refresh coroutine itself.

So there seems to clearly be a paradox/contradiction here, conflating async vs non-async credentials.

Keep in mind that this only happens if the first HTTP request attempt fails with a retry status code, which is unlikely because the before_request also proactively does a refresh; but it is technically possible for the case to happen in the small window of time before the credentials expire, and quite nasty to diagnose when it does happen intermittently.

Fix:

                     async with self._refresh_lock:
-                        await self._loop.run_in_executor(
-                            None, self.credentials.refresh, auth_request
-                        )
+                        await self.credentials.refresh(auth_request)

Future(?)

A more fancy approach would be to conditionally await the refresh OR run a background thread, depending on whether async vs non-async credentials are used; but that's a bit more involved and might be more of a "nice to have" / future improvement.

Right now though the refresh call appears to definitely be broken, causing intermittently bizarre behavior when this code tries to trigger a refresh, so hopefully the quick fix can be applied.

(I don't have time to submit a PR for this, but maybe this info will help someone)

@eevelweezel
Copy link

eevelweezel commented Jun 21, 2023

Verified this is still a problem with v. 2.20.0.

@inyutin
Copy link

inyutin commented Oct 16, 2024

Verified this is still a problem with v2.32.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

7 participants