Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Convert entire image proxy route async #3388
Convert entire image proxy route async #3388
Changes from all commits
4fd417d
ef3ef35
d20b69e
a7f2ae8
1d6ebd5
d56376d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Sorry I'm missing a lot of sync-to-async context since this is the first such PR that I'm reviewing. Why do we not use
async def
, but need ansync_to_async
for tallies?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.
We are using
async def
, unless I unintentionally missed it somewhere.async def
is only necessary when awaiting, though.sync_to_async
is necessary to prevent context issues in the Redis client, which assumes that only one thread is every touching the connection pool.sync_to_async
creates a safe context for such synchronous functions. Relevant to Django ORM as well. From Django's documentation:As an additional point of clarification, Django's own async cache support just wraps the synchronous versions of functions with
sync_to_async
, rather than using fully async clients. In the future we could switch to an asynchronous Redis client (like https://github.com/Andrew-Chen-Wang/django-async-redis, but I'm not sure that it's 100% stable), but for now it's not necessary and Django's own caching behaviour would still use thesync_to_async
adapter approach for this anyway.So the reason here is to create a new async version of
tallies.incr
that we can call later withawait
.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.
Let me know if I answered your question or if I misunderstood. The Django documentation on async adaption is a good read and important for understanding how and when it's necessary to adapt synchronous functions to an async context.
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.
This can't use
get_new_loop
becauseget_new_loop
is intentionally function scoped, whereas the session loop, as its name implies, is scoped to the session, and therefore cannot depend on the function scoped fixture.