Re: [PATCH] attempt connects in parallel for IPv6-capable builds

[Junio C Hamano <gitster@pobox.com>]

Eric Wong <normalperson@yhbt.net> writes:

> getaddrinfo() may return multiple addresses, not all of which
> are equally performant.  In some cases, a user behind a non-IPv6
> capable network may get an IPv6 address which stalls connect().
> Instead of waiting synchronously for a connect() to timeout, use
> non-blocking connect() in parallel and take the first successful
> connection.
>
> This may increase network traffic and server load slightly, but
> makes the worst-case user experience more bearable when one
> lacks permissions to edit /etc/gai.conf to favor IPv4 addresses.

Umm.  I am not sure what to think about this change--I generally do
not like a selfish "I'll try to use whatever resource given to me
to make my process go faster, screw the rest of the world" approach
and I cannot decide if this falls into that category.

I'll wait for opinions from others.

Thanks.

> Signed-off-by: Eric Wong <normalperson@yhbt.net>
> ---
>  connect.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 104 insertions(+), 14 deletions(-)
>
> diff --git a/connect.c b/connect.c
> index fd7ffe1..74d2bb5 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -14,6 +14,42 @@
>  static char *server_capabilities;
>  static const char *parse_feature_value(const char *, const char *, int *);
>  
> +#ifdef SOCK_NONBLOCK /* Linux-only flag */
> +#  define GIT_SOCK_NONBLOCK SOCK_NONBLOCK
> +#else
> +#  define GIT_SOCK_NONBLOCK 0
> +#endif
> +
> +static int socket_nb(int domain, int type, int protocol)
> +{
> +	static int flags = GIT_SOCK_NONBLOCK;
> +	int fd = socket(domain, type | flags, protocol);
> +
> +	/* new headers, old kernel? */
> +	if (fd < 0 && errno == EINVAL && flags != 0) {
> +		flags = 0;
> +		fd = socket(domain, type, protocol);
> +	}
> +
> +	/* couldn't use SOCK_NONBLOCK, set non-blocking the old way */
> +	if (flags == 0 && fd >= 0) {
> +		int fl = fcntl(fd, F_GETFL);
> +
> +		if (fcntl(fd, F_SETFL, fl | O_NONBLOCK) < 0)
> +			die_errno("failed to set nonblocking flag\n");
> +	}
> +
> +	return fd;
> +}
> +
> +static void set_blocking(int fd)
> +{
> +	int fl = fcntl(fd, F_GETFL);
> +
> +	if (fcntl(fd, F_SETFL, fl & ~O_NONBLOCK) < 0)
> +		die_errno("failed to clear nonblocking flag\n");
> +}
> +
>  static int check_ref(const char *name, unsigned int flags)
>  {
>  	if (!flags)
> @@ -351,6 +387,9 @@ static int git_tcp_connect_sock(char *host, int flags)
>  	struct addrinfo hints, *ai0, *ai;
>  	int gai;
>  	int cnt = 0;
> +	nfds_t n = 0, nfds = 0;
> +	struct pollfd *fds = NULL;
> +	struct addrinfo **inprogress = NULL;
>  
>  	get_host_and_port(&host, &port);
>  	if (!*port)
> @@ -371,20 +410,76 @@ static int git_tcp_connect_sock(char *host, int flags)
>  		fprintf(stderr, "done.\nConnecting to %s (port %s) ... ", host, port);
>  
>  	for (ai0 = ai; ai; ai = ai->ai_next, cnt++) {
> -		sockfd = socket(ai->ai_family,
> -				ai->ai_socktype, ai->ai_protocol);
> -		if ((sockfd < 0) ||
> -		    (connect(sockfd, ai->ai_addr, ai->ai_addrlen) < 0)) {
> +		size_t cur;
> +		int fd = socket_nb(ai->ai_family, ai->ai_socktype,
> +					ai->ai_protocol);
> +		if (fd < 0) {
>  			strbuf_addf(&error_message, "%s[%d: %s]: errno=%s\n",
>  				    host, cnt, ai_name(ai), strerror(errno));
> -			if (0 <= sockfd)
> -				close(sockfd);
> -			sockfd = -1;
>  			continue;
>  		}
> +
> +		if (connect(fd, ai->ai_addr, ai->ai_addrlen) < 0 &&
> +					errno != EINPROGRESS) {
> +			strbuf_addf(&error_message, "%s[%d: %s]: errno=%s\n",
> +				host, cnt, ai_name(ai), strerror(errno));
> +			close(fd);
> +			continue;
> +		}
> +
>  		if (flags & CONNECT_VERBOSE)
> -			fprintf(stderr, "%s ", ai_name(ai));
> -		break;
> +			fprintf(stderr, "%s (started)\n", ai_name(ai));
> +
> +		nfds = n + 1;
> +		cur = n;
> +		ALLOC_GROW(fds, nfds, cur);
> +		cur = n;
> +		ALLOC_GROW(inprogress, nfds, cur);
> +		inprogress[n] = ai;
> +		fds[n].fd = fd;
> +		fds[n].events = POLLIN|POLLOUT;
> +		fds[n].revents = 0;
> +		n = nfds;
> +	}
> +
> +	/*
> +	 * nfds is tiny, no need to limit loop based on poll() retval,
> +	 * just do not let poll sleep forever if nfds is zero
> +	 */
> +	if (nfds > 0)
> +		poll(fds, nfds, -1);
> +
> +	for (n = 0; n < nfds && sockfd < 0; n++) {
> +		if (fds[n].revents & (POLLERR|POLLHUP))
> +			continue;
> +		if (fds[n].revents & POLLOUT) {
> +			int err;
> +			socklen_t len = (socklen_t)sizeof(err);
> +			int rc = getsockopt(fds[n].fd, SOL_SOCKET, SO_ERROR,
> +						&err, &len);
> +			if (rc != 0)
> +				die_errno("getsockopt errno=%s\n",
> +					strerror(errno));
> +			if (err == 0) { /* success! */
> +				sockfd = fds[n].fd;
> +				ai = inprogress[n];
> +			}
> +		}
> +	}
> +
> +	/* cleanup */
> +	for (n = 0; n < nfds; n++) {
> +		if (fds[n].fd != sockfd)
> +			close(fds[n].fd);
> +	}
> +	free(inprogress);
> +	free(fds);
> +
> +	if (sockfd >= 0) {
> +		enable_keepalive(sockfd);
> +		set_blocking(sockfd); /* the rest of git expects blocking */
> +		if (flags & CONNECT_VERBOSE)
> +			fprintf(stderr, "%s done.\n", ai_name(ai));
>  	}
>  
>  	freeaddrinfo(ai0);
> @@ -392,11 +487,6 @@ static int git_tcp_connect_sock(char *host, int flags)
>  	if (sockfd < 0)
>  		die("unable to connect to %s:\n%s", host, error_message.buf);
>  
> -	enable_keepalive(sockfd);
> -
> -	if (flags & CONNECT_VERBOSE)
> -		fprintf(stderr, "done.\n");
> -
>  	strbuf_release(&error_message);
>  
>  	return sockfd;