py-mine / mcstatus

A Python library for checking the status of Minecraft servers
https://mcstatus.readthedocs.io
Apache License 2.0
483 stars 37 forks source link

`JavaServer.lookup`: Uncaught exception `dns.resolver.LifetimeTimeout` #648

Closed elmurato closed 1 year ago

elmurato commented 1 year ago

Hi there 👋

In Home Assistant we are facing the problem, that sometimes dns.resolver.LifetimeTimeout is raised while using the lookup function of JavaServer. It looks like resolving the SRV record times out, but mcstatus doesn't catch this exception here:

try:
    host, port = mcstatus.dns.resolve_mc_srv(host, lifetime=lifetime)
except (dns.resolver.NXDOMAIN, dns.resolver.NoAnswer):
    if default_port is None:
        raise ValueError(
            f"Given address '{address}' doesn't contain port, doesn't have an SRV record pointing to a port,"
            " and default_port wasn't specified, can't parse."
        )
    port = default_port

Am I right? Thanks in advance.

kevinkjt2000 commented 1 year ago

You are correct. Mcstatus does not catch every possible exception. Welcome to Python programming where you may need to catch some exceptions and deal with them in your own code.

Is there a reason you think this particular exception is an issue for mcstatus? Or is the DNS SRV record or DNS server perhaps configured incorrectly on your end?

elmurato commented 1 year ago

Thank you for your fast answer 👍

For sure I can catch that exception within Home Assistant. But I think it would be cleaner, if mcstatus would handle that: 1) Since it's possible to set the lifetime argument, I was expecting that the API can also handle it. 2) In my case dnspython is an indirect dependency, therefore I would like to not start working with it. 3) If I only enter a hostname (which is not a SRV record) without a port, the SRV record resolver is always running and this exception is then potentially raised. So this affects both, hostname only addresses and SRV records.

In the linked problem the server of the user was offline at that time. I don't know if it was actually a SRV record or not. If this information is important please let me know. I can get that info.

ItsDrike commented 1 year ago

In the linked problem the server of the user was offline at that time. I don't know if it was actually a SRV record or not. If this information is important please let me know. I can get that info.

Hm, in vast majority of cases, the DNS records are managed by the servers of the domain registrar, like Couldflare, Namecheap, etc. not by that domain's owner's server. That said, it's not impossible to configure your DNS like this, and even to have the DNS handled by the same machine that runs the minecraft server itself, it's just a very uncommon thing to do. If the client's server is set up to handle the actual DNS queries for their domain, it crashing could indeed cause this error though.

  1. Since it's possible to set the lifetime argument, I was expecting that the API can also handle it.

You can always set a higher timeout if you need to, for example you can use JavaServer.lookup(address, timeout=8) for 8 second timeouts instead of the default 3 second ones. But yeah, mcstatus doesn't abstract away these underlying errors, even though it lets you control this aspect of dnspython direcly. More on why below.

  1. In my case dnspython is an indirect dependency, therefore I would like to not start working with it.

Indeed, what mcstauts could do is create wrapper exception clases to throw instead of the ones from dnspython, however we would pretty much end up just mimicing all of the possible exceptions from dnspython ourselves, and with that, we'd probably also want to handle a bunch of other things, like exceptions from the socket connections, etc.

Mcstatus currently doesn't have any "custom" exception classes, since it's pretty complex to do all of this. Mimicing and handling all of these possible exceptions would be a really hard task for us. We did actually already discuss figuring out what exceptions can occur where, and potentially groupping them or representing them with our own exceptions, but it's not an easy task, due to how dynamic these libraries can be, and all of the various errors that can occur from a lot of different places.

Even if mcstatus did have their own exception wrapper classes here, we still couldn't raise something like ServerOfflineError, as that's not necessarily what this error means. In this case it just means it wasn't able to get the SRV record in time from the configured DNS server. That configured DNS server was then likely trying to reach to the domain's DNS server, which it seems that in your case was the client's server directly.

However as I mentioned before, this is a very rare case to see, usually, the DNS servers for domains are handled by the domain registrars, and their servers are very fast, and even provide fallbacks, so this is a very rare issue to see, and these DNS servers failing wouldn't necessarily even mean the server was offline. It's possible that there wasn't any SRV record for that domain anyway, and that we could connect with the address given directly, but we need to show that this lookup failed, as it is an exceptional state here. This means that abstracting it to ServerOfflineError just wouldn't be correct here, we'd need McstatusDnsLifetimeTimeout exception, which would just be a wrapper to the dnspython exception you got.

I don't believe creating wrappers for all possible exceptions is really viable for mcstuats though, and so, you'll probably just have to handle this one yourself manually, but I am open to suggestions.

  1. If I only enter a hostname (which is not a SRV record) without a port, the SRV record resolver is always running and this exception is then potentially raised. So this affects both, hostname only addresses and SRV records.

You might not need to use lookup if you know you're using an address that is already directly referring to the server running that minecraft server. Instead, you can initialize the JavaServer class yourself: s = mcstatus.JavaServer("myhost"), leaving the port alone to assume the default value of 25565, or s = mcstatus.JavaServer("myhost", 25565), specifying it directly, in case you'd want a non-default value)

The lookup function is here to mimic what minecraft client would do, if you entered this address into the multiplayer direct connect field, as it would perform this SRV lookup.

kevinkjt2000 commented 1 year ago
  1. ...

Because of that, it is not possible for mcstatus to handle the exception gracefully. An API user could set a ridiculously low timeout and cause that lifetime exception to happen non-stop and what would mcstatus do in those cases? If you said raise an exception, you are correct! Exceptional behavior warrants raising exceptions.

  1. ...

Python itself has plenty of exceptions in the base libraries, which are also indirect dependencies. I see no reason behind this argument on not catching exceptions in the application code given that Python is designed like this.

  1. ...

Same as ItsDrike mentioned: skip using lookup if SRV is not desired. Also, hostnames are not addresses. They have to be resolved with DNS before they become addresses. The lookup function mimics the client as ItsDrike already described. Unless you provide the IP address and port, there will be at least an A or AAAA record query for hostnames. The lookup function does an SRV query before an A or AAAA query.

Closing this, as I see no issue within mcstatus to fix. The solution here is to increase the timeout, improve the DNS setup that is perhaps faulty, avoid using the lookup function, or catch the exception in the application code.

elmurato commented 1 year ago

Thank you all for your detailed feedback, much appreciated 👍

I would really like wrapper exceptions in mcstatus, but I also understand that this is a lot of effort with less benefit for you.

I'll try to catch the exception within Home Assistant then. I just wanted to check whether it can be handled in mcstatus first.

And regarding not using lookup: I am relying on it since I don't know what the user enters in the address field. It can be anything (including SRV). And that's why lookup is perfect for that, same behavior as in the Minecraft client.