Convert entire image proxy route async #3388
Conversation
openverse-bot
added
🟧 priority: high
sarayourfriend
force-pushed
the
add/async-image-proxy
branch
from
785ad48 to
ef3ef35
Compare
| @pytest.fixture(scope="session") | ||
| def session_loop() -> asyncio.AbstractEventLoop: | ||
| loop = asyncio.new_event_loop() | ||
| yield loop | ||
| loop.close() |
There was a problem hiding this comment.
This can't use get_new_loop because get_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.
| @pytest.fixture | ||
| def photon_get(session_loop): | ||
| """ | ||
| Run ``image_proxy.get`` and wait for all tasks to finish. | ||
| """ | ||
|
|
||
| def do(*args, **kwargs): | ||
| try: | ||
| res = session_loop.run_until_complete(_photon_get(*args, **kwargs)) | ||
| return res | ||
| finally: | ||
| tasks = asyncio.all_tasks(session_loop) | ||
| for task in tasks: | ||
| session_loop.run_until_complete(task) | ||
|
|
||
| yield do |
There was a problem hiding this comment.
This is a side effect of using fire_and_forget, which is a benefit of async code, but does make testing slightly more complex. We need to ensure those tasks settle before asserting anything about the side effects of the function.
We can easily abstract this into a re-usable pattern for the future, something like a task_aware_async_to_sync that accepts a function and returns a session-looped sync version of it for testing. The abstraction isn't necessary now, so I've avoided it, but just wanted to note that fire_and_forget makes this pattern necessary, and we'll need to reuse it anywhere we're testing code paths that utilise that behaviour.
There was a problem hiding this comment.
With fire_and_forget do you mean the use of the do_not_wait_for function or is it something else?
On the other hand, it's great that these tests didn't need to change too much with the fixture! 💯
There was a problem hiding this comment.
Yes, I meant do_not_wait_for. I wrote this comment before Olga and I had decided on the better name for it. Everything is still accurate in the comment aside from the name.
| @@ -88,9 +122,10 @@ def get( | |||
|
|
|||
| logger = parent_logger.getChild("get") | |||
| tallies = django_redis.get_redis_connection("tallies") | |||
| tallies_incr = sync_to_async(tallies.incr) | |||
There was a problem hiding this comment.
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 an sync_to_async for tallies?
There was a problem hiding this comment.
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:
The reason this is needed in Django is that many libraries, specifically database adapters, require that they are accessed in the same thread that they were created in. Also a lot of existing Django code assumes it all runs in the same thread, e.g. middleware adding things to a request for later use in views.
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 the sync_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 with await.
There was a problem hiding this comment.
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.
|
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @krysal Excluding weekend1 days, this PR was ready for review 3 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2. @sarayourfriend, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
| @pytest.fixture | ||
| def photon_get(session_loop): | ||
| """ | ||
| Run ``image_proxy.get`` and wait for all tasks to finish. | ||
| """ | ||
|
|
||
| def do(*args, **kwargs): | ||
| try: | ||
| res = session_loop.run_until_complete(_photon_get(*args, **kwargs)) | ||
| return res | ||
| finally: | ||
| tasks = asyncio.all_tasks(session_loop) | ||
| for task in tasks: | ||
| session_loop.run_until_complete(task) | ||
|
|
||
| yield do |
There was a problem hiding this comment.
With fire_and_forget do you mean the use of the do_not_wait_for function or is it something else?
On the other hand, it's great that these tests didn't need to change too much with the fixture! 💯
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
|
That sentry issue is fine. It only looks like a new issue because requests threw an |
Fixes
Fixes #2789 by @sarayourfriend
Description
This PR converts
image_proxy::getto an asynchronous function. This makes the entirethumbnailsroute fully asynchronous, meaning we finally start to gain benefits from running the application under ASGI. We should see increased thumbnail throughput after this (though, it might not be noticeable because we probably won't receive more traffic), but as a result, we should be able to lower the number of thumbnail tasks in the mean time.We should assume this will increase CPU and memory usage, however, as now each thumbnails worker will be able to handle multiple requests, which will require more context switching (read: increased memory). This is really the first "real" async benefit we get though, because the thumbnails route is heavily asynchronous. The vast majority of time our servers are handling thumbnail requests they're merely waiting for an upstream response! That means they're wasting cycles that could be spent serving (preparing) additional requests to the proxy.
This PR will finalise the Django ASGI project as far as the currently created issues. However, I want to clarify something about the project, and have done so in the project thread.
Testing Instructions
Everything should continue to work as before. You should be able to follow the testing instructions in #3020, but on top of that you should also test to confirm that Redis tallying, in particular the "fire and forget" parts, are still working. You can do this via the redis cli:
just exec cache redis-cli -n 3and reading the relevant keys before/after a request to ensure they've incremented as expected.Check out the new implementation, ask questions as usual. Confirm the unit test changes are only enough to make the existing unit tests work with the async implementation, but that they do not change the actual function of the tests (i.e., that the overall behaviour has stayed the same because the unit tests still test the same outcomes).
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin