aio-libs / aiobotocore

asyncio support for botocore library using aiohttp
https://aiobotocore.rtfd.io
Apache License 2.0
1.14k stars 179 forks source link

Fix detected blocking call inside the event loop #1120

Closed chemelli74 closed 1 month ago

chemelli74 commented 2 months ago

Description of Change

Fix detected blocking call inside the event loop. Reference HomeAssistant latest build:

2024-06-12 09:40:41.237 WARNING (MainThread) [homeassistant.util.loop] Detected blocking call to listdir inside the event loop by integration 'aws'
at homeassistant/components/aws/notify.py, line 38: return await session.get_available_regions(service) (offender: /usr/local/lib/python3.12/site-pa
ckages/botocore/loaders.py, line 311: api_versions = os.listdir(full_dirname)), please create a bug report at https://github.com/home-assistant/core
/issues?q=is%3Aopen+is%3Aissue+label%3A%22integration%3A+aws%22

Assumptions

none

Checklist for All Submissions

Checklist when updating botocore and/or aiohttp versions

thehesiod commented 2 months ago

sessions should be cached for the life of use so if this is a major issue the session should be kept around for a longer time. really botocore needs to speed this up on their end because it's way too slow

chemelli74 commented 2 months ago

Until then we have a fix anyway. Maybe we add a comment about that.

thehesiod commented 2 months ago

I'm hesitant of doing this as run_in_executor triggers a thread pool to be created (if you haven't called it before) which can cause overhead. Further the executor should be configurable..this really opens a can of worms.

bdraco commented 2 months ago

It does looks like load_service_model calls are cached, but it does do blocking I/O in the event loop the first time its created. Ideally we would only call it in the executor the first time it needs to be created.

run_in_executor being called with None (default executor) seems fine to me as we do it all over the place in aiohttp and that is the prescribed way to run blocking code https://docs.python.org/3/library/asyncio-dev.html#running-blocking-code

chemelli74 commented 1 month ago

Any feedback @thehesiod ? Thank you in advance

thehesiod commented 1 month ago

so this seems like an issue with the aws component, why doesn't it do the session/client creation during async_setup instead? That seems like the correct fix no?

thehesiod commented 1 month ago

like i was saying, it should cache the session/client there, or if you don't want to hold onto the client, perhaps just pre-warm up the botocore cache

thehesiod commented 1 month ago

actually, this is invalid as botocore is not thread safe, with this you could have two requests, running in two different threads, trying to initialize the cache at the same time. closing

bdraco commented 1 month ago

@chemelli74 Thanks for trying to fix this. This problem must be solved eventually (unless the intent is WONTFIX). Please open an issue instead to track this in the long term. Thanks.

thehesiod commented 1 month ago

ya let me know if I missed something, I truly believe this is a botocore issue. The fact they're reading a ton of JSON files is unsustainable. probably should be done on the fly per service when you use it...and even then should be in a post-processed state, like their build should generate python code based on those json files.

thehesiod commented 1 month ago

so I looked a little into this, it seems like they use an instance_cache which basically just sets an attribute on the class, so presumably worst case scenario with this fix is you'd initialize the cache multiple times. However I really want to avoid adding threads to this library if possible. Looks like there are some async file alternatives like https://github.com/qweeze/uring_file for debian, I'd be open to something like that.

chemelli74 commented 1 month ago

What about pathlib ? Is used all over

thehesiod commented 1 month ago

that uses aiofiles which just uses a threadpool again: https://github.com/Tinche/aiofiles/tree/main/src/aiofiles

thehesiod commented 1 month ago

i guess at the aiofiles level though it doesn't matter. But it sounds like there's a better solution

chemelli74 commented 1 month ago

i guess at the aiofiles level though it doesn't matter. But it sounds like there's a better solution

If you point me towards the solution you prefer, I'll try to work on it. I really would like to fix this issue and it's effect on Home Assistant.

EDIT: Opened issue on botocore repo

chemelli74 commented 3 weeks ago

Here the official answer from botocore repo: https://github.com/boto/botocore/issues/3222

What can we do to progress then ?

jakob-keller commented 3 weeks ago

edit: sorry, this is basically what @thehesiod suggested https://github.com/aio-libs/aiobotocore/pull/1120#issuecomment-2187180231. I also think it's the obvious solution for that HomeAssistant specific issue.

This might be a crazy idea, but if

  1. botocore caches the stuff it loads from disk in global state (not thread-local)
  2. aiobotocore uses the same cache
  3. HomeAssistant supports set-up hooks for the AWS integration

then

  1. The set up code in HomeAssistant could pre-populate the botocore cache in a non-blocking fashion, i.e. via asyncio.to_thread()
  2. Eventual calls to aiobotocore would no longer block and leverage the pre-warmed cache

This would be a fix over at HomeAssistant. What do you think?

bdraco commented 3 weeks ago

This doesn’t seem like a Home Assistant specific issue. Any asyncio application is going to have its event loop blocked. The only difference is that Home Assistant is aware of its loop being blocked where-as another application may not be able to detect it.

jakob-keller commented 3 weeks ago

botocore (or us?) could provide some sort of synchronuous helper function to pre-populate the cache. Home Assistant or anyone else could then choose to run it as part of their setup routine, e.g. via asyncio.to_thread() and no one else would be affected. Does that make sense?

Btw, there are Python platforms that lack multithreading support. That's why I think @thehesiod has a point in not resorting to helper threads in this project unless absolutely necessary.

thehesiod commented 3 weeks ago

this isn't an aiobotocore problem I don't think, but a botocore problem, or do you need to go through an async call to get to the cache warmup? secondly they can already do this themselves without the need for a helper

chemelli74 commented 3 weeks ago

this isn't an aiobotocore problem I don't think, but a botocore problem.

Why you think so ? This is a async issue and botocore is sync. The main point of aiobotocore is to transform botocore to async, so IMHO it's a issue in this library .

thehesiod commented 3 weeks ago

I suppose if we find a good async file API to use then it would be an async problem. but then that wouldn't require an init function. in terms of an init function that would equally apply to sync, however you'll then potentially run into locking issues

chemelli74 commented 1 day ago

@thehesiod how can we progress ? I really want to find a fix

thehesiod commented 1 day ago

@chemelli74 suggest doing research of a good async API for aiobotocore to use. This will be a huge project though. Furthermore there are better ways to warm-up aiobotocore, if homeassistant wants to do that I already gave some suggestions like: https://github.com/aio-libs/aiobotocore/pull/1120#issuecomment-2187180231. They should init and keep the aioboto clients around instead of creating them on the fly

thehesiod commented 1 day ago

this is really a solved issue, we due the same thing, like in aiohttp server using the on_startup callback to init your resources