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.

Sign up for GitHub

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

fix(auth): fixup aiohttp v3.3.0 compat #717

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(auth): fixup aiohttp v3.3.0 compat #717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions auth/gcloud/aio/auth/session.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ async def get(
self, url: str, headers: Optional[Mapping[str, str]],
timeout: float, params: Optional[Mapping[str, Union[int, str]]],
stream: bool,
auto_decompress: bool,
auto_decompress: Optional[bool],
) -> Response:
pass

Expand Down Expand Up @@ -200,7 +200,7 @@ async def get( # type: ignore[override]
timeout: Timeout = 10,
params: Optional[Mapping[str, Union[int, str]]] = None,
stream: Optional[bool] = None,
auto_decompress: bool = True,
auto_decompress: Optional[bool] = None,
) -> aiohttp.ClientResponse:
if not isinstance(timeout, aiohttp.ClientTimeout):
timeout = aiohttp.ClientTimeout(total=timeout)
Expand All @@ -211,11 +211,21 @@ async def get( # type: ignore[override]
'this argument is only used by SyncSession',
stream,
)

# TODO: in aiohttp v3.9.0, session.get(..) learned the
# auto_decompress argument. Once our minimum bound is >=3.9.0,
# update this block to avoid patching the prviate session
# attribute.
# pylint: disable=protected-access
orig = self.session._auto_decompress
if auto_decompress is not None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to use an asyncio.Lock here? Suppose you are running 2 coros concurrently using the same session, but one uses auto_decompress=True and the other one the default value. The first coro modifies self.session._auto_decompress, then does the await and the loop hands control to the second coro, which does not enter the if clause. Since the session is shared, the second coro will be executing with the _auto_decompress set by the first coro, even if that's not the expected behaviour

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic with a lock should be:

  1. If overriding session._auto_decompress then we have to acquire the lock and release it after we restore it
  2. If not overriding it, we still need to acquire it (to make sure there's no other coro that is overriding session._auto_decompress), but we can release it immediately (before doing the actual http call). This is to avoid hurting performance

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh shoot, that's a great point. In fact, I think it's even worse -- we need to acquire and use the lock unconditionally, since if we release the lock before calling get(), it'd still be possible for a future request to override the method before the first get() actually makes it to the usage of that flag.

Damn, this would be a massive performance regression.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Darn it, you are right, I hadn't thought about that use case. It's pretty bad

Copy link
Contributor

@fbalboaDialpad fbalboaDialpad Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of implementing something that uses a copy of the session instead of modifying the current one. It's kinda hacky but we could have a single session with auto_decompress, and we create and save a second one for requests with auto_decompress=False on demand (if there's ever a request that uses this param, I expect most apps will not even use this), in order to reuse connections.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheKevJames thoughts on the above?

self.session._auto_decompress = auto_decompress
resp = await self.session.get(
url, headers=headers,
timeout=timeout, params=params,
auto_decompress=auto_decompress,
)
self.session._auto_decompress = orig

await _raise_for_status(resp)
return resp

Expand Down Expand Up @@ -339,9 +349,9 @@ async def get(
timeout: float = 10,
params: Optional[Mapping[str, Union[int, str]]] = None,
stream: bool = False,
auto_decompress: bool = True,
auto_decompress: Optional[bool] = None,
) -> Response:
if not auto_decompress and not stream:
if auto_decompress is False and not stream:
warnings.warn(
'the requests library always decompresses responses when '
'outside of streaming mode; when auto_decompress is '
Expand Down
2 changes: 1 addition & 1 deletion auth/poetry.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion auth/pyproject.rest.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ classifiers = [

[tool.poetry.dependencies]
python = ">= 3.8, < 4.0"
# aiohttp = ">= 3.9.0, < 4.0.0"
# aiohttp = ">= 3.3.0, < 4.0.0"
backoff = ">= 1.0.0, < 3.0.0"
chardet = ">= 2.0, < 6.0"
# See https://cryptography.io/en/latest/api-stability/#deprecation
Expand Down
2 changes: 1 addition & 1 deletion auth/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ classifiers = [

[tool.poetry.dependencies]
python = ">= 3.8, < 4.0"
aiohttp = ">= 3.9.0, < 4.0.0"
aiohttp = ">= 3.3.0, < 4.0.0"
backoff = ">= 1.0.0, < 3.0.0"
chardet = ">= 2.0, < 6.0"
# See https://cryptography.io/en/latest/api-stability/#deprecation
Expand Down
Loading