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

Fix resolution for host names starting with a decimal digit. #117

Closed
wants to merge 1 commit into from

Conversation

marcioapm
Copy link

No description provided.

@marcioapm
Copy link
Author

Hi @s-ludwig
We recently upgraded to 0.8.4 and this is biting us hard.
If you merge it, could you also tag a new version as soon as possible?

@s-ludwig
Copy link
Member

s-ludwig commented Jan 9, 2019

The fix in this form is unfortunately breaking the other direction (IP addresses are also supposed to work with use_dns == true). But as a quick/correct alternative I'd suggest to keep the condition and simply wrap the if branch in a try {} catch (SocketException) and let it fall through to the else branch if an exception was caught. That would will mean that a transient exception will be thrown for DNS names starting with a digit, but unless the exception is printed (.toString) at least that shouldn't have a measurable performance impact.

I'll tag a (pre) release as soon as this is merged.

@marcioapm
Copy link
Author

Well, in that case I think we should use a statically initialized regex to correctly match ip4 and ip6 addresses. It will be more efficient and more correct than the exception allocation and overhead - what do you think?

@s-ludwig
Copy link
Member

s-ludwig commented Jan 9, 2019

Not sure about regex because of the compile-time overhead, but yes that would definitely be better. From an effort/gain point of view the exception based approach is hard to beat, though. The exception allocation and stack unwinding overhead should be dwarfed by the actual DNS lookup anyway.

@marcioapm
Copy link
Author

How about this test?
Pretty efficient and seems like it could work, based on the facts that : is not a valid name character and a TLD name cannot start with a digit.
See https://stackoverflow.com/questions/9071279/number-in-the-top-level-domain

bool canBeName(const(char)[] host) {
    import std.string : indexOf, lastIndexOf;
    import std.ascii : isDigit;

    if (host.indexOf(':') != -1);
        return false;
    const tld = host.lastIndexOf('.') + 1;
    return !tld || tld >= host.length || !host[tld].isDigit;
}

if (!use_dns || !canBeName(host)) {
  // numeric ...
} else {
  // name ...
}

@marcioapm
Copy link
Author

Alternatively we can bypass std.socket, call getaddrinfo directly with AI_NUMERICHOST and interpret the return code, doing the correct thing and avoiding the exception overhead - how do you feel about that?

@s-ludwig
Copy link
Member

The canBeName approach looks fine, AFAICS. I'd stay with std.socket if possible, as I was hoping to be able to slowly start reusing parts from Phobos (which is also why scoped InternetAddress instances are used within eventcore instead of a custom struct).

@s-ludwig
Copy link
Member

I took the idea and pushed a solution with a slightly modified IPv4 branch: #121

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.

None yet

2 participants