Skip to content
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

[asyncio] Blocking call to import_module in rdata.py #1083

Closed
elmurato opened this issue May 6, 2024 · 5 comments
Closed

[asyncio] Blocking call to import_module in rdata.py #1083

elmurato opened this issue May 6, 2024 · 5 comments
Labels
Future A longer term issue, possibly addressed in the future. Not a Bug

Comments

@elmurato
Copy link

elmurato commented May 6, 2024

Describe the bug
The rdata.py module of dnspython does blocking I/O while importing other modules. The line causing this issue seams to be here:

mod = import_module(

Warning shown in Home Assistant Core:

Detected blocking call to import_module inside the event loop by integration 'minecraft_server' at homeassistant/components/minecraft_server/api.py, line 78: self._server = await JavaServer.async_lookup(self._address) (offender: /usr/local/lib/python3.12/site-packages/dns/rdata.py, line 660: mod = import_module()

Is this something which can be fixed in dnspython?

Background information:
I'm the maintainer of the Home Assistant Core integration Minecaft Server, which uses mcstatus as a dependency, which again uses dnspython as a dependency.

Home Assistant Core Issue:
home-assistant/core#116854

Minecraft Server integration call:
https://github.com/home-assistant/core/blob/673bbc13721af0024dd9c4c64ff7f2fe7ac735ff/homeassistant/components/minecraft_server/api.py#L78

mcstatus call:
https://github.com/py-mine/mcstatus/blob/ffefe37cc28da9e98f7028ffc6ebc9d924d2a714/mcstatus/dns.py#L67

Similar issues in Home Assistant Core:
home-assistant/core#116425 (comment)

To Reproduce
Add Minecraft Server integration to Home Assistant Core and check the system logs (I know this isn't very helpful for you).

Context (please complete the following information):

  • dnspython version 2.4.2
  • Python version 3.12
  • OS: Home Assistant OS
@rthalley
Copy link
Owner

rthalley commented May 6, 2024

rdata.py dynamically loads the modules for the various DNS types, so this is the intended behavior. This import happens once per DNS RdataType the first time the type is used. I don't understand what Home Assistant is doing well enough to know why such an import would be a problem. I don't think there is any good workaround for this, alas, though perhaps you could call dns.rdata.get_rdata_class() to preload the classes you are using before entering the event loop.

As calling import_module() is legit python for sync and async code, I don't regard this as a bug so it's not something I'm likely to change in the near term, though it's possible that some future release of dnspython could abandon dynamic type loading.

@bdraco
Copy link

bdraco commented May 7, 2024

Home Assistant examines sys.modules when an import call is made in the event loop. If the module is not already loaded, it will raise this warning because the import call has to open files containing the Python code, which does blocking I/O in the loop for the duration the code is being loaded. Since Home Assistant loads thousands of py files at startup, all that blocking I/O can cause quite a bit of instability and timeouts because the loop will get starved for run-time. For example, some modules like https://github.com/project-chip/connectedhomeip can take up to 10s to load, which will cause any async code running a timeout to fail unexpectedly. While this case is likely only blocking for a few milliseconds, that can add up quickly when many integrations and modules are being loaded at startup.

@rthalley
Copy link
Owner

rthalley commented May 7, 2024

Thanks for the explanation! I'll keep thinking about alternatives for the longer term.

@rthalley rthalley added the Future A longer term issue, possibly addressed in the future. label May 25, 2024
@rthalley
Copy link
Owner

So, I thought more about this and fiddled with some code. I've decided I don't want to solve this specific problem in the distribution as so far it is only really relevant to the Home Assistant case, and there is a workaround for that (code below). Alas this workaround is not useful for the main case where people have problems with dynamic loading, which is "freezing into an executable by static analysis of the source".

Here's the preloading code I suggested above:

import dns.rdata
import dns.rdataclass
import dns.rdatatype

def load_all_types():
    for rdtype in dns.rdatatype.RdataType:
        if not dns.rdatatype.is_metatype(rdtype) or rdtype == dns.rdatatype.OPT:
            dns.rdata.get_rdata_class(dns.rdataclass.IN, rdtype)

load_all_types()

@bdraco
Copy link

bdraco commented Jun 16, 2024

I updated the linked issue with the suggested workaround home-assistant/core#116854 (comment)

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Future A longer term issue, possibly addressed in the future. Not a Bug
Projects
None yet
Development

No branches or pull requests

3 participants