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

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

[Eric Wong]

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.

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;
-- 
EW

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

[Junio C Hamano]

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;

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

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> 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.

No problem, I can also make it cheaper for servers to handle
aborted connections in git-daemon:

standalone:

  1) use recv with MSG_PEEK or FIONREAD to determine if there's
     readable data in the socket before forking (and avoid
     forking for zero-bytes-written connections)

  2) use TCP_DEFER_ACCEPT in Linux and dataready filter in FreeBSD
     for standalone git-daemon to delay accept()

inetd:

  3) suppress die("The remote end hung up unexpectedly")
     if no bytes are read at all

At some point in the future, I would love to have git-daemon implement
something like IDLE in IMAP (to avoid having clients poll for updates).
Perhaps the standalone changes above would make sense there, too.

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

[Junio C Hamano]

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().

I'd assume that you are not solving a hypothetical problem, but you
may (at least sometimes) have to reach outside world from such a
network environment.  I further assume that git_tcp_connect() is not
the only activity you do from such a network, and other network
activities are similarly affected.

How do you work around the same issue for connections that do not go
through git_tcp_connect()?  The same issue would affect Git traffic
going over git-remote-curl, and also your usual Web browser traffic,
no?

What I am getting at is if it is saner to solve the issue like how
curl(1) solves it with its -4/-6 command line options, e.g. by
adding a pair of configuration variables "net.ipv[46] = true/false".

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

[Torsten Bögershausen]

On 2016-01-29 04.04, Junio C Hamano wrote:
> 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().
> 
> I'd assume that you are not solving a hypothetical problem, but you
> may (at least sometimes) have to reach outside world from such a
> network environment.  I further assume that git_tcp_connect() is not
> the only activity you do from such a network, and other network
> activities are similarly affected.
> 
> How do you work around the same issue for connections that do not go
> through git_tcp_connect()?  The same issue would affect Git traffic
> going over git-remote-curl, and also your usual Web browser traffic,
> no?
> 
> What I am getting at is if it is saner to solve the issue like how
> curl(1) solves it with its -4/-6 command line options, e.g. by
> adding a pair of configuration variables "net.ipv[46] = true/false".

(Please don't do parallel connects as a default)

I like the -4 / -6

Out of my head these kind of setups are not desired.
(getaddrinfo returns an address and the the following connect fails)
But it may be hard to fully avoid them in practice,
and some RFC have been written,  this may be a starting point:

https://www.rfc-editor.org/rfc/rfc5014.txt

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

[Johannes Sixt]

Am 29.01.2016 um 02:41 schrieb Eric Wong:
> Junio C Hamano <gitster@pobox.com> wrote:
>> 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.
>
> No problem, I can also make it cheaper for servers to handle
> aborted connections in git-daemon:
>
> standalone:
>
>    1) use recv with MSG_PEEK or FIONREAD to determine if there's
>       readable data in the socket before forking (and avoid
>       forking for zero-bytes-written connections)
>
>    2) use TCP_DEFER_ACCEPT in Linux and dataready filter in FreeBSD
>       for standalone git-daemon to delay accept()
>
> inetd:
>
>    3) suppress die("The remote end hung up unexpectedly")
>       if no bytes are read at all
>
> At some point in the future, I would love to have git-daemon implement
> something like IDLE in IMAP (to avoid having clients poll for updates).
> Perhaps the standalone changes above would make sense there, too.

Before you submit a patch in that direction (or resubmit the patch under 
discussion here), could you please find someone to test your patch on 
Windows first? A lot of the infrastructure mentioned may not be 
available there or may not work as expected. (I admit that I'm just 
hand-waving, I haven't tested your patch.)

Thanks,
-- Hannes

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

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> 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().
> 
> I'd assume that you are not solving a hypothetical problem, but you
> may (at least sometimes) have to reach outside world from such a
> network environment.  I further assume that git_tcp_connect() is not
> the only activity you do from such a network, and other network
> activities are similarly affected.

Right.  I only recently started building kernels for this
netbook with IPv6 support.   Most of my traffic from this host
is git/ssh-tunneled and the host I ssh to does not have an AAAA
record.

> How do you work around the same issue for connections that do not go
> through git_tcp_connect()?  The same issue would affect Git traffic
> going over git-remote-curl, and also your usual Web browser traffic,
> no?

So this is a new issue for me, and I don't use a browser much.

I noticed the curl(1) tool didn't have the problem with http.

Looking at curl, it actually has limited parallelization
(see HAPPY_EYEBALLS_TIMEOUT timeout handling in lib/connect.c
of git://github.com/bagder/curl.git).  It starts a parallel
connection on a different family if the first address takes
longer than 200ms.  This does not seem true of our libcurl
usage, though...

I tried implementing what curl does in connect.c, but it was
complicated for me (or more likely, I'm easily confused :x).
It was much easier for me to start everything up in parallel
as I did in my original patch.

> What I am getting at is if it is saner to solve the issue like how
> curl(1) solves it with its -4/-6 command line options, e.g. by
> adding a pair of configuration variables "net.ipv[46] = true/false".

Yes, a -4/-6 patch is on the way for command-line switches.  I'm
not sure if a config file variable is a good idea since it could
be overlooked and cause non-obvious conflicts with /etc/gai.conf
settings.

Of course, I would rather have something transparent to the user
like curl(1)-style happy eyeballs parallelization if it could be
implemented in a straightforward way.