Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

gethostbyaddr() causes long loading times #154

Closed
massl opened this Issue Apr 17, 2012 · 10 comments

Comments

Projects
None yet
3 participants

massl commented Apr 17, 2012

As mentioned by a person in the php.net comments in this function:

"
Just wanted to let everyone know that gethostbyaddr() takes more than 20 seconds to respond if the IP address is not listed in DNS.

So be careful if you are going to use this function in the production environment. You or your users may not be able to get the response from the server before the timeout occurs. Slow website only makes people very frustrated.

You shouldn't use this function at all unless you have a very good reason to do so (taking logs? then save ip addresses instead and resolve them later by running a batch or something.)"

Exactly this one fucked up my site ;)

You really should only use the function if the $address is not a valid IPv4. Otherwise is makes no sense and is just senseless overhead.

massl commented Apr 17, 2012

Maybe a fix would also be:

    $addresses = gethostbynamel($address);
    if(empty($addresses)) {
        throw new SteamCondenserException("Cannot resolve $address");
    }
    if (sizeof($addresses) > 1 || $addresses[0] != $address) {
        foreach($addresses as $address) {
            $this->hostNames[]   = $address;
            $this->ipAddresses[] = $address;
        }
    }
Owner

koraktor commented Apr 17, 2012

Good catch, I'll have a look at it.

But feel free to open a pull request once you have a solid solution.

massl commented Apr 17, 2012

Well. You can use this code or really do a check if it's a valid IPv4 and leave that gethostbynamel(), too.

Just let me know if I should code it when you don't want to do it ;)

massl commented Apr 17, 2012

Actually I don't know how to use the github stuff :)

But this one works fine:

    $ipv4Pattern = '/^(?:(?:25[0-5]|2[0-4][0-9]|(?:(?:1[0-9])?|[1-9]?)[0-9])\.){3}(?:25[0-5]|2[0-4][0-9]|(?:(?:1[0-9])?|[1-9]?)[0-9])$/';
    if (!preg_match($ipv4Pattern, $address)) {
        $addresses = gethostbynamel($address);
        if(empty($addresses)) {
            throw new SteamCondenserException("Cannot resolve $address");
        }
        foreach($addresses as $address) {
            $this->hostNames[]   = $address;
            $this->ipAddresses[] = $address;
        }
    } else {
        $this->hostNames = $this->ipAddresses = array($address);
    }
    $this->ipAddress = $this->ipAddresses[0];
Owner

koraktor commented Apr 17, 2012

Ok... I could verify that slow behavior on Windows, but my OS X box works fine. Maybe that's why I didn't notice this problem yet.

Essentially, I'd go for your first solution, the second one isn't wrong, but gethostbynamel() (or the OS) seems to be smart enough to handle IP addresses even if there's no DNS record for them. So I don't think adding this overhead is really necessary.

damianb commented Apr 17, 2012

don't use regex for ipv4 formatting verification under php, use filter_var() with the appropriate filter setting and flags (FILTER_VALIDATE_IP and FILTER_FLAG_IPV4 or FILTER_FLAG_IPV6, etc.).

Owner

koraktor commented Apr 17, 2012

@damianb Thanks for the input, didn't know that yet.

But as said above, I wouldn't verify the address at all and instead let gethostbynamel() handle it, as far as I can tell this works reliable and fast. But feel free to prove me wrong...

@ghost ghost assigned koraktor Apr 17, 2012

massl commented Apr 17, 2012

@damianb : Thanks for this one. One thing more I've learned :)

@koraktor : The first "solution" didn't work. It seems that $this->hostNames and/or $this->ipAddresses must be set to at least the one host. So it did not query anything ;)

So here are the two versions:

    // Solution one:
    $addresses = gethostbynamel($address);
    if(empty($addresses)) {
        throw new SteamCondenserException("Cannot resolve $address");
    }
    if (sizeof($addresses) > 1 || $addresses[0] != $address) {
        foreach($addresses as $address) {
            $this->hostNames[]   = $address;
            $this->ipAddresses[] = $address;
        }
    } else {
        $this->hostNames = $this->ipAddresses = array($address);
    }




    // Solution two:
    if (filter_var($address, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4) != $address) {
        $addresses = gethostbynamel($address);
        if(empty($addresses)) {
            throw new SteamCondenserException("Cannot resolve $address");
        }
        foreach($addresses as $address) {
            $this->hostNames[]   = $address;
            $this->ipAddresses[] = $address;
        }
    } else {
        $this->hostNames = $this->ipAddresses = array($address);
    }

koraktor added a commit to koraktor/steam-condenser-php that referenced this issue Apr 17, 2012

Don't reverse lookup server host names
PHP's gethostbyaddr() is slow and the information isn't that valuable.

See koraktor/steam-condenser#154
Owner

koraktor commented Apr 17, 2012

I decided to just scrap reverse lookups entirely. It's not an essential feature, PHP's own implementation is slow and I won't add the overhead of an own implementation or an additional library dependency.

@koraktor koraktor closed this Apr 17, 2012

damianb commented Apr 17, 2012

@koraktor that would probably be best. If anything, just tell the user to go whois the IP, or plug it into robtex or something. Plenty of tools available for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment