-
Notifications
You must be signed in to change notification settings - Fork 92
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
base: master
Are you sure you want to change the base?
Conversation
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: |
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.
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
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.
The logic with a lock should be:
- If overriding
session._auto_decompress
then we have to acquire the lock and release it after we restore it - 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
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.
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.
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.
Darn it, you are right, I hadn't thought about that use case. It's pretty bad
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.
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.
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.
@TheKevJames thoughts on the above?
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.
approved: aiohttp compatibility
Avoid new API usage, introduce ability to avoid stomping over configured
session-level
auto_decompress
setting.