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

Token refresh uses blocking I/O in async function #245

Open
anuraaga opened this issue Feb 7, 2024 · 2 comments
Open

Token refresh uses blocking I/O in async function #245

anuraaga opened this issue Feb 7, 2024 · 2 comments
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: cleanup An internal cleanup or hygiene concern.

Comments

@anuraaga
Copy link

anuraaga commented Feb 7, 2024

Bug Description

Token refresh in the async Client class uses the requests.Request transport of the google auth library

resp = await self._client.post(
url, headers=headers, json=data, raise_for_status=True
)

This means it is doing blocking I/O on the asyncio loop. At the minimum, the fetch seems like it needs to be delegated to a thread with asyncio.to_thread.

For reference, google-auth-library-python does have some baking experimental APIs for async

https://github.com/googleapis/google-auth-library-python/blob/main/google/auth/transport/_aiohttp_requests.py
https://github.com/googleapis/google-auth-library-python/blob/main/google/auth/_default_async.py

However, assuming using the experimental APIs was ok, it's probably still not ready for general use because at least, impersonated credentials don't yet have an async version (in certain token refresh logic I've written, I use the async APIs for normal creds and to_thread for impersonated, but it seems too complicated for this library).

https://github.com/googleapis/google-auth-library-python/blob/main/google/auth/impersonated_credentials.py

Example code (or command)

No response

Stacktrace

No response

Steps to reproduce?

  1. ?
  2. ?
  3. ?
    ...

Environment

  1. OS type and version:
  2. Python version:
  3. AlloyDB Python Connector version:

Additional Details

No response

@anuraaga anuraaga added the type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. label Feb 7, 2024
@jackwotherspoon
Copy link
Collaborator

Hi @anuraaga thanks for raising an issue on the AlloyDB Python Connector 😄

I've been waiting for the google auth library to GA it's async interface for credentials but still not sure the timeline for when that will be. We want to keep this library stable and thus not rely on those APIs just yet.

We can probably look at writing our own version of to_thread as I believe the function was only added in Python 3.9 while we support Python 3.8+ with the Python Connector.

Will look at hopefully getting to a fix for this shortly, meanwhile we are always open to external contributions if you would like to take a pass at it, just let me know. 😄

Thanks again for raising this issue!

@jackwotherspoon jackwotherspoon added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Feb 7, 2024
@enocom enocom added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. type: cleanup An internal cleanup or hygiene concern. and removed type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Mar 4, 2024
@jackwotherspoon
Copy link
Collaborator

Looks like there may be a way to do this by looking at credentials.token_state googleapis/google-auth-library-python#1368

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Development

No branches or pull requests

3 participants