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

Change valid-hostname check to socket.gethostbyname #202

Closed
wants to merge 2 commits into from
Closed

Change valid-hostname check to socket.gethostbyname #202

wants to merge 2 commits into from

Conversation

PerchunPak
Copy link

This is a more accurate way than regex, because it is do request to DNS server, and trying to get number ip from domain. So this have almost 100% accurate.
BUT it is triggers to not registered domains, about which haven't any information on DNS servers. Anyway it is more accurate than regex, because previous one triggered to www.

Copy link
Contributor

@Iapetus-11 Iapetus-11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bad idea because that is a blocking io function that is called when you create a MinecraftServer or MinecraftBedrockServer object. This is an issue because asyncio code will freeze whenever a MinecraftServer or MinecraftBedrockServer object is created. If anything, it should be included in the lookup classmethods.

Copy link
Contributor

@ItsDrike ItsDrike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

48e90b1 is unacceptable, the entire reason for that check was to prevent from making invalid instances of these server classes, that means they need to happen each time that instance gets initialized, that's why it was in __init__. Moving these checks to lookup brings this issue back, since it allows for that invalid initialization that lead us to add this validation in the first place.

Specifically this issue was pointed out in #189 which clearly shown how unreadable were these error messages without checks like these occurring on initialization to prevent these oddities completely.

I assume you added this logic as a response to lapetuses comment, but I don't think this is what they meant by it. The point of that comment was just to say that making a DNS request like this synchronously each time the server instance gets initialized isn't good, since it's a blocking synchronous function which doesn't play nicely with async. However that doesn't mean that check isn't important, it just means that making a DNS request to run it isn't ideal.

The fact that there needs to be a non-blocking synchronous mechanism to prevent passing invalid host IPs/names that's running directly on initialization is the reason for using that regex there. The only alternative would be to add checks to basically every function for some bool flag like "is_initialized" and have a new function with both sync and async versions, that performs this initialization check, but I honestly just don't think that's worth the hassle since doing that would completely break backwards compatibility for little benefit.

Using hostnames that would result in a DNS lookup error isn't a huge issue anyway, because the moment we would try to actually establish that connection, we would see a somewhat clear error from the socket standard library about failed name resolution.

Copy link
Collaborator

@kevinkjt2000 kevinkjt2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for trying to improve something in mcstatus! And welcome, as I see this is your first contribution!

Unfortunately, this is something that has already been decided against doing. #190 (comment)

# Check if `host` is valid, this is a more accurate way than regex or like this.
# `gethostbyname` makes request to dns, and get number ip, so this have 100% accuracy
# BUT it is triggers to not registered domains, which can be ignored
gethostbyname(host)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SRV records can be used for Minecraft servers without a corresponding A record, which would break this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants