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

Resolve IPv6 address takes too much time #696

Closed
zh1029 opened this issue Aug 5, 2019 · 5 comments · Fixed by #1096
Closed

Resolve IPv6 address takes too much time #696

zh1029 opened this issue Aug 5, 2019 · 5 comments · Fixed by #1096
Assignees
Labels

Comments

@zh1029
Copy link

zh1029 commented Aug 5, 2019

We have nss-mdns (http://0pointer.de/lennart/projects/nss-mdns/#documentation) deployed in our system and publish IPv6 address with DNS via avahi-publish. The problem that we realize redis connect via redis-cli takes much time(several seconds). The root cause is hiredis first looks for IPv4 address of the DNS, than looks for IPv6 address. but due to system doesn't publish IPv4 address, It takes time(timeout) to failed to get IPv4 address.

static int _redisContextConnectTcp(redisContext *c, const char *addr, int port,
                                   const struct timeval *timeout,
                                   const char *source_addr) {
    ......
    snprintf(_port, 6, "%d", port);
    memset(&hints,0,sizeof(hints));
    hints.ai_family = AF_INET;
    hints.ai_socktype = SOCK_STREAM;

    /* Try with IPv6 if no IPv4 address was found. We do it in this order since
     * in a Redis client you can't afford to test if you have IPv6 connectivity
     * as this would add latency to every connect. Otherwise a more sensible
     * route could be: Use IPv6 if both addresses are available and there is IPv6
     * connectivity. */
    if ((rv = getaddrinfo(c->tcp.host,_port,&hints,&servinfo)) != 0) {
         hints.ai_family = AF_INET6;
         if ((rv = getaddrinfo(addr,_port,&hints,&servinfo)) != 0) {
            __redisSetError(c,REDIS_ERR_OTHER,gai_strerror(rv));
            return REDIS_ERR;
        }
    }
    ......
}

So can hints.ai_family be set AF_UNSPEC instead of try IPv4 and IPv6 in order?

@mnunberg
Copy link
Contributor

mnunberg commented Aug 9, 2019

Perhaps we can add this as an option?

@zh1029
Copy link
Author

zh1029 commented Aug 15, 2019

I don't understand the reason behind here to try IPv4 first, then IPv6 in order. Doesn't it always working no matter get IPv4 or IPv6 whatever configured?

@ac000
Copy link

ac000 commented Feb 19, 2020

The normal way to do this is as zh1029 says and use AF_UNSPEC with a single call to getaddrinfo()

Underneath, getaddrinfo() will race to find working IPv4 and IPv6 addresses, preferring IPv6 (i.e IPv6 addresses will be the first in the list as returned). I believe this is part of the 'Happy Eyeballs' protocol.

@michael-grunder
Copy link
Collaborator

I've not heard of this before, but it looks interesting.

It seems reasonable to add this logic as long as we can do it without breaking too many things.

@zuiderkwast
Copy link
Contributor

Good morning!

This has been discussed recently in the Redis main repo. DNS-lookup has been added in redis-cli. See redis/redis#11151 and redis/redis#10436.

TL;DR for redis-cli, this was decided and implemented:

  • It was decided to use AF_UNSPEC by default.
  • It was not acceptable to prefer IPv4 over IPv6. According to standards, the default should rather be to prefer IPv6. It is also something the user can configure in /etc/gai.conf.
  • Since not all users have access to configure /etc/gai.conf, options -4 and -6 were added to redis-cli to prefer IPv4 or IPv6 respectively.

For hiredis, I suggest we do something similar.

  • We can add options for redisConnectWithOptions to prefer IPv4, IPv6 or unspecified.
  • We can leave the default to prefer IPv4 over IPv6 for backward compatibilty, to be really safe.
  • ... or, if you (@michael-grunder) are willing to take the risk, we can change the default to AF_UNSPEC. I think it is a better default in general.

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

Successfully merging a pull request may close this issue.

5 participants