Add aiohttp client sharing

[sarayourfriend]

Fixes

Fixes #2788 by @sarayourfriend

Description

Shares the aiohttp client within a given event loop.

I've also switched the "OpenverseASGIHandler", which was subclassing Django's ASGI handler and required us to write a bespoke get_asgi_application function, duplicating Django's built-in initialisation work in our code. Instead, we can follow a clearer middleware/handler pattern that calls to a parent application. This works very well and is so clear that I think we could even publish it as a Django extension in the future, provided there isn't one already. The Django project rejected a proposal to add lifecycle handling to Django itself, so there is a need for a library to handle it transparently.

Along with that small refactor, I also used a single consistent name for the "application" uvicorn runs. This reduces the number of places we need to check for the environment to decide which application to run to just inside asgi.py, instead of both there and run.py. To make sure we're always referencing the correct object to register lifecycle events against, I've exported the lifecycle handler/application as its own singleton.

Testing Instructions

Checkout the branch and run just build web in case you don't have the dependencies added in #3011. For good measure, delete your Redis volume to prevent cached dead links. For extra good measure, run just recreate and forget about fiddly details :)

Run the API and open the logs with just logs web. Make a search request with the filter_dead=True and confirm you see a log about creating a new client session because none exists (grep logs with just logs web | grep aiohttp to filter the logs down to the relevant ones). Make repeat requests, related requests, and requests for different queries. Confirm that in the logs you can see messages about re-using the same session. Finally, test the shutdown behaviour works as expected by making an arbitrary change to any file in the API code. In the web container's logs you should see uvicorn reload the application, and in the process, a log indicating how many shutdown handlers were executed. Specifically you should see it log that only 1 handler was executed, for the single, re-used aiohttp session: "Executed 1 handler(s) before shutdown".

As above, you can filter the logs using grep to make this easier:

$ just logs web | grep lifecycle_handler
just dc logs -f web
env COMPOSE_PROFILES="api,ingestion_server,frontend,catalog" docker-compose -f docker-compose.yml logs -f web
openverse-web-1  | [2023-11-03 00:28:59,601 - conf.lifecycle_handler... -  75][INFO] [none] Executed 0 handler(s) before shutdown.

To force test that sessions are used per loop, change the decorator on check_dead_links.__init__._make_head_requests to @async_to_sync(force_new_loop=True) so that a new loop is created every time the function is called. You should now see a log from api.utils.aiohttp about creating a new session each time you make a new search request.

Each time you make a search request, you must ensure that dead link checking runs. The only consistent way to do this is to use a unique query each time. I've tested this by cycling through queries for "birds", "cat", and "run". Result tags are a good place to easily find new queries to run that will require dead link checking. Just keep in mind that dead link masking and caching will prevent repeated queries from re-running dead link checks, and you need to work around that to exercise the dead link checking route while testing. Alternatively, you could use the redis CLI to remove all cached dead links, but to me that seemed way more tedious that just using a new query term each time, so I didn't even try it and don't have any advice for attempting it.

Checklist

• My pull request has a descriptive title (not a vague title likeUpdate index.md).
• My pull request targets the default branch of the repository (main) or a parent feature branch.
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added or updated tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no visible errors.
• [N/A] I ran the DAG documentation generator (if applicable).

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[krysal]

LGTM! Thanks for the detailed instructions and tests.

[AetherUnbound]

This looks great! No blocking comments, I was able to perform all of the testing steps mentioned except for the shutdown step. For whatever reason, changes that I make to the API code locally aren't causing the API service to reload 🤔 I have DJANGO_DEBUG_ENABLED=True and ENVIRONMENT=development, not sure why that might be happening, but the rest looks great!

[AetherUnbound]

Out of curiosity, where did you piece together that this class was necessary, and what to include within it?

[sarayourfriend]

I read in the Django bug tracker that ASGI lifecycle wasn't supported. Somehow I didn't come across this project, though, which I wonder if we should use instead of our custom implementation: https://github.com/illagrenan/django-asgi-lifespan

[sarayourfriend]

Update: yeah, I'm going to go ahead and change this PR to use that package so that we don't have to do a refactor later to remove our custom implementation.

[sarayourfriend]

Ah! I remember why I originally did not use that package: it only supports uvicorn directly, but not under gunicorn workers. That's not an issue for us anymore, so we can use it.

It doesn't have a lot of activity, but it has comprehensive unit tests and by using Django signals, gets around the need to implement custom weak ref handling (Django signal handler refs are weak by default).

[sarayourfriend]

Okay, that's done in the latest commit: e4dc958

It actually was a pretty nice way to clean things up. Rather than registering each session's close and needing to manage it, we can benefit from the _SESSIONS gathering point and register a single signal handler that closes all the sessions.

I had to refactor the test fixture as well, and almost thought it was finally going to break completely, until I realised I'd come across a bug in the upstream library, for which I've submitted a patch: illagrenan/django-asgi-lifespan#80

It doesn't currently affect us, and shouldn't in the foreseeable future, because we don't need the startup lifecycle event (and worst-case scenario, we can still call the startup event without awaiting it and just tolerate a log at the end of the tests complaining that it was never awaited).

In any case, I think the end result is a lot cleaner and easier to follow, and easier to extend into the future.

[AetherUnbound]

Nice simplification!

[AetherUnbound]

Great test!

[krysal]

@AetherUnbound I believe you need ENVIRONMENT=local to see the API service reloading; that is what I have in my .env file, though admittedly, I find it a bit odd and think it should be set to development as in your case 🤔

[AetherUnbound]

@AetherUnbound I believe you need ENVIRONMENT=local to see the API service reloading; that is what I have in my .env file, though admittedly, I find it a bit odd and think it should be set to development as in your case 🤔

Thanks, I'll give that a shot! FWIW that appears to be the default:

openverse/api/env.template

Line 8 in 19e6e4b

ENVIRONMENT=development

[AetherUnbound]

@krysal indeed that was it! Do you think we should change the default environment to local in the env.template file?

[sarayourfriend]

Development is overloaded. It was used as the deployed "staging" environment for a long time. The original ASGI PR already changed it to local instead of development, in line with the ingestion server and the frontend. Looks like I forgot to add it to the template, so I'll do that in this PR now 👍.