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

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

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

Conversation

TheKevJames
Copy link
Member

Avoid new API usage, introduce ability to avoid stomping over configured
session-level auto_decompress setting.

Avoid new API usage, introduce ability to avoid stomping over configured
session-level `auto_decompress` setting.
# 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

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?

Copy link
Contributor

@eddiedialpad eddiedialpad left a comment

Choose a reason for hiding this comment

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

approved: aiohttp compatibility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants