Dinnerbone / mcstatus

A Python class for checking the status of an enabled Minecraft server
http://dinnerbone.com/minecraft/tools/status/
1.11k stars 146 forks source link

To replace sync 'dns.resolver.resolve' with `dns.asyncresolver` #211

Closed ba1dr closed 2 years ago

ba1dr commented 2 years ago

Across the code for the asynchronous functions please replace usage of sync dns resolving.

Example - MinecraftServer.async_query:

answers = dns.resolver.resolve(host, "A")

can be replaced with:

from dns import asyncresolver
...
answers = await asyncio.wait_for(
    asyncresolver.resolve(host, "A"),
    timeout=timeout)
ItsDrike commented 2 years ago

DNS resolving is pretty fast and usually only happens once, on MinecraftServer.lookup (sync), this is an alternative constructor method and there is no async alternative because we generally don't expect people to make too many new instances like this, and so there was never a need to make this asynchronous, but we certainly could add an async lookup version there, which would make the SRV DNS lookup asynchronously and pass the values to __init__ afterwards.

The more complicated part to address is the query function, which currently performs the DNS resolving each time it's called. This is somewhat unnecessary and it may be a good idea to resolve this IP ahead of time. I was able to think of 4 basic ways in which we could address this:

I don't think there are any other cases of us performing DNS lookups like these. Though I do have another solution for the problem of query, which actually resolves a much deeper problem about how our UDP async connections don't resolve the address and expect IP only.

With the sync TCP versions, we can get away with passing host and port easily, because socket module implements socket.create_connection function, which performs the DNS resolving for us, and for the async TCP connections, there's asyncio.make_connection, which can also perform this resolving. However with the async UDP connections, we're using asyncio_dgram.connect and this does not run the DNS resolving, which is very weird since it makes our Connection classes inconsistent and annoying to deal with. Instead we should simply look into how asyncio.make_connection is handling DNS resolving and replicate that for the UDP async connections directly. The same issue occurs with synchronous UDP connections, since we're just initializing socket.socket directly, which doesn't perform any DNS resolving.