[PATCH] Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0

From: "Shawn O. Pearce" <redacted>

If the user doesn't want to use the dumb HTTP protocol, she may
set GIT_CURL_FALLBACK=0 in the environment before invoking a Git
protocol operation. This is mostly useful when testing against
servers that are known to not support the dumb protocol. If the
smart service detection fails the client should not continue with
dumb behavior, but instead provide accurate HTTP failure data.

Signed-off-by: Shawn O. Pearce <redacted>
---
 remote-curl.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 4a0927e..2f91128 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -17,7 +17,8 @@ struct options {
 	unsigned progress : 1,
 		followtags : 1,
 		dry_run : 1,
-		thin : 1;
+		thin : 1,
+		fallback : 1;
 };
 static struct options options;
 

@@ -115,7 +116,8 @@ static struct discovery* discover_refs(const char *service)
 	http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);
 
 	/* try again with "plain" url (no ? or & appended) */
-	if (http_ret != HTTP_OK && http_ret != HTTP_NOAUTH) {
+	if (options.fallback && http_ret != HTTP_OK
+	    && http_ret != HTTP_NOAUTH) {
 		free(refs_url);
 		strbuf_reset(&buffer);
 

@@ -868,6 +870,12 @@ int main(int argc, const char **argv)
 	options.verbosity = 1;
 	options.progress = !!isatty(2);
 	options.thin = 1;
+	options.fallback = 1;
+
+	if (getenv("GIT_CURL_FALLBACK")) {
+		char *fb = getenv("GIT_CURL_FALLBACK");
+		options.fallback = *fb != '0';
+	}
 
 	remote = remote_get(argv[1]);
 

-- 
1.7.12.1.512.g9b230e6

On Wed, Sep 19, 2012 at 7:55 PM, Shawn O. Pearce [off-list ref] wrote:

From: "Shawn O. Pearce" <redacted>

I can't explain why git send-email did this. I obviously didn't need
the extra From header here. format-patch didn't write it to the patch
file, it was injected by send-email. My .git/config is pretty simple,
the name/email are derived from there:

  [user]
	name = Shawn O. Pearce
	email = spearce@spearce.org

Ick. I really don't want to debug this right now so I'm just going to
pretend it wasn't written.

On Wed, Sep 19, 2012 at 07:55:53PM -0700, Shawn O. Pearce wrote:

From: "Shawn O. Pearce" <redacted>

If the user doesn't want to use the dumb HTTP protocol, she may
set GIT_CURL_FALLBACK=0 in the environment before invoking a Git
protocol operation. This is mostly useful when testing against
servers that are known to not support the dumb protocol. If the
smart service detection fails the client should not continue with
dumb behavior, but instead provide accurate HTTP failure data.

I have been looking into this recently, as well. GitHub does not allow
dumb http at all these days, so transient errors on the initial smart
contact can cause us to fall back to dumb, and end up reporting a
totally useless 403 Forbidden error.  I guess Google Code has a similar
issue.

Note that it is not really do not "fall back to dumb"; we detect the
dumb nature from the response. It is really "fall back to trying the URL
without the query string, because there are some servers that cannot
handle it". With your patch, we might still end up performing a dumb
transfer.

I think what you're doing here is sane, because you have to turn it on
manually, and thus there are no possible backwards compatibility issues.
But it might be nice to make things work better out of the box. Here are
two client-side changes I've been toying with:

  1. If both smart and dumb requests fail, report the error for the
     smart request. Now that smart-http clients are common, I'd expect
     most http servers to be smart these days. Of course I don't have
     any sort of numbers to back this up (nor am I sure how to get them;
     obviously big sites like GitHub and Google Code do a lot of
     traffic, but who knows how many one-off repo-on-a-generic-web-host
     sites still exist?).

     An alternative would be to simply be more verbose, and mention that
     we tried to fallback and list both failures (or we could do this
     with just "fetch -v").

  2. Be more discerning about which errors will cause a fallback.
     Something like "504 Gateway Timeout" should not give a fallback.
     The problem is that you are really guessing at what kinds of http
     errors you are going to get from a dumb server when you try the
     smart URL. I dug back into the list thread that spawned the "retry
     without query string" patch (703e6e7).

     The thread is here:

       http://thread.gmane.org/gmane.comp.version-control.git/137609

     If you read the thread, it turns out that the problem in this case
     (which is the only reported case I could find in the archive) is
     that the server was misconfigured to treat _anything_ with a query
     string as a gitweb URL. And then it got fixed pretty much
     immediately.

     So as far as we know, there may be zero servers for which this
     fallback is actually doing anything useful.

I'm tempted to just reverse the logic. Try the request with the query
string and immediately fail if it doesn't work. For the few (if any)
people who are hitting a server that will not serve the dumb file in
that case, add a "remote.*.dumbhttp" setting that will turn off smart
completely as a workaround.

That would serve the (presumed) majority who are using smart http,
everyone using dumb http on a reasonably-configured server, and still
allow an easy workaround for people with badly configured servers.

What do you think?

---
 remote-curl.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

If we do go this route, the patch itself looks fairly obvious, although:

This can just be:

  options.fallback = git_env_bool("GIT_CURL_FALLBACK", 1);

Fewer lines, and you get all of the true/false parsing for free.

-Peff

On Wed, Sep 19, 2012 at 08:22:58PM -0700, Shawn O. Pearce wrote:

On Wed, Sep 19, 2012 at 7:55 PM, Shawn O. Pearce [off-list ref] wrote:

quoted

From: "Shawn O. Pearce" <redacted>

I can't explain why git send-email did this. I obviously didn't need
the extra From header here. format-patch didn't write it to the patch
file, it was injected by send-email. My .git/config is pretty simple,
the name/email are derived from there:

  [user]
	name = Shawn O. Pearce
	email = spearce@spearce.org

Ick. I really don't want to debug this right now so I'm just going to
pretend it wasn't written.

I was looking at the send-email author comparison code a month or three
ago, and I remember noticing that it totally fails to canonicalize
before comparing.  Without even looking at it again, I'm fairly sure
that it thinks '"Shawn O. Pearce"' and 'Shawn O. Pearce' (i.e., with and
without the quotes) are different, and therefore author != committer.

In your case it is particularly egregious because the quotes are
introduced (correctly) by format-patch, so it is not even like you have
configured two different versions of your name. 

I think the same bug exists for different rfc2047 encodings of a name
with non-ascii characters. Fixing both would involve canonicalizing the
names before comparing. I wonder if it would be simpler to just compare
the email addresses and ignore the names entirely.

-Peff

Introduce http.$url_prefix.dumbhttpfallback configuration variables
so that users do not have to set GIT_DUMB_HTTP_FALLBACK environment
depending on which remote they are talking with.

Signed-off-by: Junio C Hamano <redacted>
---
 remote-curl.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 6 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index f25cf3c..44544c7 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -855,6 +855,46 @@ static void parse_push(struct strbuf *buf)
 	free(specs);
 }
 
+struct dumb_fallback_cb {
+	const char *url;
+	int value;
+};
+
+static int dumb_fallback_cb(const char *key, const char *value, void *cb_)
+{
+	struct dumb_fallback_cb *cb = cb_;
+	int i, len;
+
+	/* Is this "http.$url.dumbHttpFallback"? */
+	if (prefixcmp(key, "http."))
+		return 0;
+	len = strrchr(key, '.') - key;
+	if (len <= 5)
+		/* we found the dot at the end of "http." */
+		return 0;
+	key += 5; /* skip over http. part */
+	len -= 5;
+	if (strcmp(key + len, ".dumbhttpfallback"))
+		return 0;
+
+	/* Does the $url part match? */
+	for (i = 0; i < len; i++)
+		if (cb->url[i] != key[i])
+			return 0;
+	cb->value = git_config_bool(key, value);
+	return 0;
+}
+
+static int dumb_fallback_config(const char *url)
+{
+	struct dumb_fallback_cb cb;
+
+	cb.url = url;
+	cb.value = 1; /* defaults to true */
+	git_config(dumb_fallback_cb, &cb);
+	return cb.value;
+}
+
 static const char DUMB_HTTP_FALLBACK_ENV[] = "GIT_DUMB_HTTP_FALLBACK";
 
 int main(int argc, const char **argv)

@@ -872,12 +912,6 @@ int main(int argc, const char **argv)
 	options.verbosity = 1;
 	options.progress = !!isatty(2);
 	options.thin = 1;
-	options.fallback = 1;
-
-	if (getenv(DUMB_HTTP_FALLBACK_ENV)) {
-		char *fb = getenv(DUMB_HTTP_FALLBACK_ENV);
-		options.fallback = git_config_bool(DUMB_HTTP_FALLBACK_ENV, fb);
-	}
 
 	remote = remote_get(argv[1]);
 

@@ -889,6 +923,13 @@ int main(int argc, const char **argv)
 
 	url = strbuf_detach(&buf, NULL);
 
+	if (getenv(DUMB_HTTP_FALLBACK_ENV)) {
+		char *fb = getenv(DUMB_HTTP_FALLBACK_ENV);
+		options.fallback = git_config_bool(DUMB_HTTP_FALLBACK_ENV, fb);
+	} else {
+		options.fallback = dumb_fallback_config(url);
+	}
+
 	http_init(remote, url, 0);
 
 	do {

-- 
1.7.12.1.389.g3dff30b

From: "Shawn O. Pearce" <redacted>

If the user doesn't want to use the dumb HTTP protocol, she may set
GIT_DUMB_HTTP_FALLBACK=false in the environment before invoking a
Git protocol operation. This is mostly useful when testing against
servers that are known to not support the dumb protocol. If the
smart service detection fails the client should not continue with
dumb behavior, but instead provide accurate HTTP failure data.

Signed-off-by: Shawn O. Pearce <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
 remote-curl.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 3ec474f..f25cf3c 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -17,7 +17,8 @@ struct options {
 	unsigned progress : 1,
 		followtags : 1,
 		dry_run : 1,
-		thin : 1;
+		thin : 1,
+		fallback : 1;
 };
 static struct options options;
 

@@ -115,7 +116,8 @@ static struct discovery* discover_refs(const char *service)
 	http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);
 
 	/* try again with "plain" url (no ? or & appended) */
-	if (http_ret != HTTP_OK && http_ret != HTTP_NOAUTH) {
+	if (options.fallback && http_ret != HTTP_OK
+	    && http_ret != HTTP_NOAUTH) {
 		free(refs_url);
 		strbuf_reset(&buffer);
 

@@ -853,6 +855,8 @@ static void parse_push(struct strbuf *buf)
 	free(specs);
 }
 
+static const char DUMB_HTTP_FALLBACK_ENV[] = "GIT_DUMB_HTTP_FALLBACK";
+
 int main(int argc, const char **argv)
 {
 	struct strbuf buf = STRBUF_INIT;

@@ -868,6 +872,12 @@ int main(int argc, const char **argv)
 	options.verbosity = 1;
 	options.progress = !!isatty(2);
 	options.thin = 1;
+	options.fallback = 1;
+
+	if (getenv(DUMB_HTTP_FALLBACK_ENV)) {
+		char *fb = getenv(DUMB_HTTP_FALLBACK_ENV);
+		options.fallback = git_config_bool(DUMB_HTTP_FALLBACK_ENV, fb);
+	}
 
 	remote = remote_get(argv[1]);
 

-- 
1.7.12.1.389.g3dff30b

"Shawn O. Pearce" [off-list ref] writes:

From: "Shawn O. Pearce" <redacted>

If the user doesn't want to use the dumb HTTP protocol, she may
set GIT_CURL_FALLBACK=0 in the environment before invoking a Git
protocol operation. This is mostly useful when testing against
servers that are known to not support the dumb protocol. If the
smart service detection fails the client should not continue with
dumb behavior, but instead provide accurate HTTP failure data.

Signed-off-by: Shawn O. Pearce <redacted>
---
 remote-curl.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

I can see how this feature may be useful, but I have to say the
external interface triply sucks.

 - If it is primarily for debugging smart HTTP, a solution with an
   environment variable without more permanent configuration
   variable would be sufficient, but then the environment variable
   is better named GIT_SMART_HTTP_DEBUG, or something no?

 - If it is useful outside the context of debugging, perhaps a per
   remote configuration variable remote.$name.$variable (or
   http.$prefix_of_server_url.$variable) might be necessary?

 - I do not see this as "fallback (to) curl"; you still talk your
   smart protocol over curl library.  "fallback to dumb http" is
   more understandable.  

In any case, I think CURL_FALLBACK was named with CURL in its name
primarily because the environment applies only to remote-curl, but
that means we cannot have any fallback logic other than the current
"smart does not work, fall back on dumb" in the future.

Here is a bit of rewrite.  [1/2] is yours but with a bit more
sensible name. [2/2] is entirely optional.


Junio C Hamano (1):
  remote-curl: make dumb-http fallback configurable per URL

Shawn O. Pearce (1):
  Disable dumb HTTP fallback with GIT_DUMB_HTTP_FALLBACK=false

 remote-curl.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

-- 
1.7.12.1.389.g3dff30b

On Wed, Sep 19, 2012 at 8:48 PM, Jeff King [off-list ref] wrote:

On Wed, Sep 19, 2012 at 07:55:53PM -0700, Shawn O. Pearce wrote:

quoted

If the user doesn't want to use the dumb HTTP protocol, she may
set GIT_CURL_FALLBACK=0 in the environment before invoking a Git
protocol operation. This is mostly useful when testing against
servers that are known to not support the dumb protocol. If the
smart service detection fails the client should not continue with
dumb behavior, but instead provide accurate HTTP failure data.

I have been looking into this recently, as well. GitHub does not allow
dumb http at all these days,

Interesting that GitHub doesn't support dumb transfer either.

so transient errors on the initial smart
contact can cause us to fall back to dumb,

Transient errors is actually what is leading me down this path. We see
about 0.0455% of our requests to the Android hosting service as these
dumb fallbacks. This means the client never got a proper smart service
reply. Server logs suggest we sent a valid response, so I am
suspecting transient proxy errors, but its hard to debug because
clients discard the error.

and end up reporting a
totally useless 403 Forbidden error.

Today I posted a change to JGit [1] to make this 406 Not Acceptable as
I think it better matches the description of the failure. It also no
longer reuses the common 403 Forbidden error that a server might
choose to return if a request was given with invalid credentials.

[1] https://git.eclipse.org/r/7847

 I guess Google Code has a similar
issue.

Yes, and {android,kernel,gerrit}.googlesource.com also only support
the smart protocol.

Note that it is not really do not "fall back to dumb"; we detect the
dumb nature from the response. It is really "fall back to trying the URL
without the query string, because there are some servers that cannot
handle it". With your patch, we might still end up performing a dumb
transfer.

Yes, the server could ignore the parameter and return a dumb info/refs
response on the initial request, forcing the client to a dumb
transfer. I forgot this case being common on a dumb server. I have
only been working with or worrying about smart servers, whose
transient failures have this info/refs request that they always
fail... giving a poor user experience.

I think what you're doing here is sane, because you have to turn it on
manually, and thus there are no possible backwards compatibility issues.
But it might be nice to make things work better out of the box. Here are
two client-side changes I've been toying with:

  1. If both smart and dumb requests fail, report the error for the
     smart request. Now that smart-http clients are common, I'd expect
     most http servers to be smart these days. Of course I don't have
     any sort of numbers to back this up (nor am I sure how to get them;
     obviously big sites like GitHub and Google Code do a lot of
     traffic, but who knows how many one-off repo-on-a-generic-web-host
     sites still exist?).

I suspect there are still a number of servers that rely on dumb
protocol to host repositories because its very simple to setup.
Breaking support for this wouldn't be a good idea.

kernel.org and eclipse.org both support the smart protocol, but I
think this is a common trend. Sites that host a lot of Git
repositories take the time to setup the smart protocol. Everyone else,
its hit-or-miss, mostly miss.

     An alternative would be to simply be more verbose, and mention that
     we tried to fallback and list both failures (or we could do this
     with just "fetch -v").

I would bet the big hosting providers would appreciate having the
failure listed in non-verbose mode. Many Git users are going to use a
hosting service like GitHub or Google Code, or work with their
organization like kernel.org for central Git hosting. Consumers of
hosted projects that are less familiar with Git will be accessing
these sites often enough.

I considered logging this failure to stderr, but didn't.

  2. Be more discerning about which errors will cause a fallback.
     Something like "504 Gateway Timeout" should not give a fallback.
     The problem is that you are really guessing at what kinds of http
     errors you are going to get from a dumb server when you try the
     smart URL.

I dug back into the list thread that spawned the "retry
     without query string" patch (703e6e7).

     The thread is here:

       http://thread.gmane.org/gmane.comp.version-control.git/137609

     If you read the thread, it turns out that the problem in this case
     (which is the only reported case I could find in the archive) is
     that the server was misconfigured to treat _anything_ with a query
     string as a gitweb URL. And then it got fixed pretty much
     immediately.

     So as far as we know, there may be zero servers for which this
     fallback is actually doing anything useful.

I'm tempted to just reverse the logic.

After re-reading that thread, it was a mistake to apply 703e6e76. We
should just revert it.

Try the request with the query
string and immediately fail if it doesn't work. For the few (if any)
people who are hitting a server that will not serve the dumb file in
that case, add a "remote.*.dumbhttp" setting that will turn off smart
completely as a workaround.

That would serve the (presumed) majority who are using smart http,
everyone using dumb http on a reasonably-configured server, and still
allow an easy workaround for people with badly configured servers.

What do you think?

Yes, this is the better idea. Revert 703e6e76 and add a new feature to
disable the smart protocol entirely as an escape hatch.

From: "Shawn O. Pearce" <redacted>

This reverts commit 703e6e76a14825e5b0c960d525f34e607154b4f7.

Retrying without the query parameter was added as a workaround
for a single broken HTTP server at git.debian.org[1]. The server
was misconfigured to route every request with a query parameter
into gitweb.cgi. Admins fixed the server's configuration within
16 hours of the bug report to the Git mailing list, but we still
patched Git with this fallback and have been paying for it since.

Most Git hosting services configure the smart HTTP protocol and the
retry logic confuses users when there is a transient HTTP error as
Git dropped the real error from the smart HTTP request. Removing the
retry makes root causes easier to identify.

[1] http://thread.gmane.org/gmane.comp.version-control.git/137609
Signed-off-by: Shawn O. Pearce <redacted>
---
 remote-curl.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 3ec474f..2359f59 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -95,7 +95,7 @@ static struct discovery* discover_refs(const char *service)
 	struct strbuf buffer = STRBUF_INIT;
 	struct discovery *last = last_discovery;
 	char *refs_url;
-	int http_ret, is_http = 0, proto_git_candidate = 1;
+	int http_ret, is_http = 0;
 
 	if (last && !strcmp(service, last->service))
 		return last;

@@ -113,19 +113,6 @@ static struct discovery* discover_refs(const char *service)
 	refs_url = strbuf_detach(&buffer, NULL);
 
 	http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);
-
-	/* try again with "plain" url (no ? or & appended) */
-	if (http_ret != HTTP_OK && http_ret != HTTP_NOAUTH) {
-		free(refs_url);
-		strbuf_reset(&buffer);
-
-		proto_git_candidate = 0;
-		strbuf_addf(&buffer, "%sinfo/refs", url);
-		refs_url = strbuf_detach(&buffer, NULL);
-
-		http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);
-	}
-
 	switch (http_ret) {
 	case HTTP_OK:
 		break;

@@ -144,8 +131,7 @@ static struct discovery* discover_refs(const char *service)
 	last->buf_alloc = strbuf_detach(&buffer, &last->len);
 	last->buf = last->buf_alloc;
 
-	if (is_http && proto_git_candidate
-		&& 5 <= last->len && last->buf[4] == '#') {
+	if (is_http && 5 <= last->len && last->buf[4] == '#') {
 		/* smart HTTP response; validate that the service
 		 * pkt-line matches our request.
 		 */

-- 
1.7.12.1.512.g9b230e6

On Wed, Sep 19, 2012 at 10:57:35PM -0700, Shawn O. Pearce wrote:

quoted

I have been looking into this recently, as well. GitHub does not allow
dumb http at all these days,

Interesting that GitHub doesn't support dumb transfer either.

Our objects are still in regular repos, and is served by fairly stock
git. The main show-stopper is that we share objects via alternates, and
we aggressively repack the alternates repos. So a dumb client would end
up being quite inefficient.

We also toyed with having mixed public/private fork networks at one
point, which would obviously necessitate disabling dumb access. But we
gave it up as too risky, as you can do all sorts of weird tricks to
convince git to disclose information about what's in the private repos
(e.g., you can reliably find out which sha1s exist, and you can lie
about which sha1s you have to ask git to create deltas against them).

Doing a shared-object store right would require marking which repo
"knows" which objects (I assume that's what Google Code does, but I
don't remember if Dave talked about that aspect at last year's
GitTogether).

quoted

so transient errors on the initial smart
contact can cause us to fall back to dumb,

Transient errors is actually what is leading me down this path. We see
about 0.0455% of our requests to the Android hosting service as these
dumb fallbacks. This means the client never got a proper smart service
reply. Server logs suggest we sent a valid response, so I am
suspecting transient proxy errors, but its hard to debug because
clients discard the error.

Yup, we see the same thing. I've tracked a few down manually to actual
things like gateway timeouts on our reverse proxy.

quoted

and end up reporting a
totally useless 403 Forbidden error.

Today I posted a change to JGit [1] to make this 406 Not Acceptable as
I think it better matches the description of the failure. It also no
longer reuses the common 403 Forbidden error that a server might
choose to return if a request was given with invalid credentials.

That might be worth doing for git-http-backend, too. It might even make
sense for the git client to recognize the 406 (and possibly the 403) and
print a more useful message.

quoted

  1. If both smart and dumb requests fail, report the error for the
     smart request. Now that smart-http clients are common, I'd expect
     most http servers to be smart these days. Of course I don't have
     any sort of numbers to back this up (nor am I sure how to get them;
     obviously big sites like GitHub and Google Code do a lot of
     traffic, but who knows how many one-off repo-on-a-generic-web-host
     sites still exist?).

I suspect there are still a number of servers that rely on dumb
protocol to host repositories because its very simple to setup.
Breaking support for this wouldn't be a good idea.

I don't think it would break on most servers, though. Even for a dumb
server, the initial error will be a useful one. It's only the
weirdly-configured ones where you get wildly different results depending
on whether the query string is there.

In other words, it is really no worse than reverting 703e6e76, and it
might be better. In the common case, you get a better error message. In
the broken-server case, we still try the fallback. So it will keep
working on a broken server without any manual intervention, whereas
reverting and adding a manual escape hatch means the user has to do
something.

BUT.

I still think it's better to revert 703e6e76, because I really do think
the broken-server case is an extreme enough minority that it is not even
worth wasting the time of clients to make a second request (especially
because the first request may very well have failed because of a network
error that causes a long timeout, and the user then has to wait double
to find out what the error is).

Of course, I have no actual data aside from reading the original thread
that led to 703e6e76, and the fact that nobody else mentioned it (not
during the time when it was broken, and not even after, when people
often still complain because they haven't upgraded yet). But who knows?
Maybe I will eat my words, and we will end up getting that data in the
form of complaints. :)

We can always switch to fallback-but-prefer-the-initial-error then. And
we'll have more data on exactly how the misconfigured servers behave.

-Peff

On Thu, Sep 20, 2012 at 10:24 AM, Jeff King [off-list ref] wrote:

On Wed, Sep 19, 2012 at 10:57:35PM -0700, Shawn O. Pearce wrote:

quoted

quoted

so transient errors on the initial smart
contact can cause us to fall back to dumb,

Transient errors is actually what is leading me down this path. We see
about 0.0455% of our requests to the Android hosting service as these
dumb fallbacks. This means the client never got a proper smart service
reply. Server logs suggest we sent a valid response, so I am
suspecting transient proxy errors, but its hard to debug because
clients discard the error.

Yup, we see the same thing. I've tracked a few down manually to actual
things like gateway timeouts on our reverse proxy.

Our reverse proxies also fail sometimes. :-)

But right now I am seeing failures in libcurl's SSL connection that
may also be causing the smart connection failures. For example this
trace, where libcurl was just not able to connect to respond to the
401 with a password. I suspect what is happening is the SSL session
dropped out of cache on our servers, and libcurl couldn't reuse the
existing SSL session. Instead of discarding the bad session and
retrying, Git aborts. I'm willing to bet modern browsers just discard
the bad session and start a new one, because clients can't assume the
remote server will be able to remember their session forever.


* Couldn't find host android.googlesource.com in the .netrc file; using defaults
* About to connect() to android.googlesource.com port 443 (#0)
*   Trying 2607:f8b0:400e:c02::52... * Connected to
android.googlesource.com (2607:f8b0:400e:c02::52) port 443 (#0)
* successfully set certificate verify locations:
*   CAfile: none
  CApath: /etc/ssl/certs
* SSL connection using RC4-SHA
* Server certificate:
*        subject: C=US; ST=California; L=Mountain View; O=Google Inc;
CN=*.googlecode.com
*        start date: 2012-08-16 12:25:39 GMT
*        expire date: 2013-06-07 19:43:27 GMT
*        subjectAltName: android.googlesource.com matched
*        issuer: C=US; O=Google Inc; CN=Google Internet Authority
*        SSL certificate verify ok.

GET /a/platform/tools/build/info/refs?service=git-upload-pack HTTP/1.1

User-Agent: git/1.7.12.1.1.g9b7ccb3
Host: android.googlesource.com
Accept: */*
Pragma: no-cache

* The requested URL returned error: 401
* Closing connection #0
* Couldn't find host android.googlesource.com in the .netrc file; using defaults
* About to connect() to android.googlesource.com port 443 (#0)
*   Trying 2607:f8b0:400e:c02::52... * Connected to
android.googlesource.com (2607:f8b0:400e:c02::52) port 443 (#0)
* successfully set certificate verify locations:
*   CAfile: none
  CApath: /etc/ssl/certs
* SSL re-using session ID
* Unknown SSL protocol error in connection to android.googlesource.com:443
* Expire cleared
* Closing connection #0
error: Unknown SSL protocol error in connection to
android.googlesource.com:443  while accessing
https://android.googlesource.com/a/platform/tools/build/info/refs?service=git-upload-pack
fatal: HTTP request failed

On Thu, Sep 20, 2012 at 04:05:03PM -0700, Shawn O. Pearce wrote:

But right now I am seeing failures in libcurl's SSL connection that
may also be causing the smart connection failures. For example this
trace, where libcurl was just not able to connect to respond to the
401 with a password. I suspect what is happening is the SSL session
dropped out of cache on our servers, and libcurl couldn't reuse the
existing SSL session. Instead of discarding the bad session and
retrying, Git aborts. I'm willing to bet modern browsers just discard
the bad session and start a new one, because clients can't assume the
remote server will be able to remember their session forever.

That's something I haven't seen. But then, I don't usually see the
client side; I just see the fallback dumb fetch in our logs, and
have occasionally followed up.

Is there a long pause while the user is typing their password?

* SSL re-using session ID
* Unknown SSL protocol error in connection to android.googlesource.com:443
* Expire cleared
* Closing connection #0
error: Unknown SSL protocol error in connection to
android.googlesource.com:443  while accessing
https://android.googlesource.com/a/platform/tools/build/info/refs?service=git-upload-pack
fatal: HTTP request failed

You could try turning off CURLOPT_SSL_SESSIONID_CACHE and seeing if that
improves it. Of course, it is probably hard to reproduce, so it would be
tough to know if that helped or not. It would also be nice if you could
dump more information on the error from the ssl library (I typically
build curl against openssl; I wonder if it could be related to using
gnutls or something).

-Peff

On Thu, Sep 20, 2012 at 10:26 PM, Jeff King [off-list ref] wrote:

On Thu, Sep 20, 2012 at 04:05:03PM -0700, Shawn O. Pearce wrote:

quoted

But right now I am seeing failures in libcurl's SSL connection that
may also be causing the smart connection failures. For example this
trace, where libcurl was just not able to connect to respond to the
401 with a password. I suspect what is happening is the SSL session
dropped out of cache on our servers, and libcurl couldn't reuse the
existing SSL session. Instead of discarding the bad session and
retrying, Git aborts. I'm willing to bet modern browsers just discard
the bad session and start a new one, because clients can't assume the
remote server will be able to remember their session forever.

That's something I haven't seen. But then, I don't usually see the
client side; I just see the fallback dumb fetch in our logs, and
have occasionally followed up.

I hadn't seen this either until I deleted the fallback code from
remote-curl.c and ran git ls-remote in a while true loop for 6 hours.
Its obviously happening though.

Is there a long pause while the user is typing their password?

No. The password comes off a credential helper that has access to it
from a credential store. There is very little lag here, under 100 ms.

quoted

* SSL re-using session ID
* Unknown SSL protocol error in connection to android.googlesource.com:443
* Expire cleared
* Closing connection #0
error: Unknown SSL protocol error in connection to
android.googlesource.com:443  while accessing
https://android.googlesource.com/a/platform/tools/build/info/refs?service=git-upload-pack
fatal: HTTP request failed

You could try turning off CURLOPT_SSL_SESSIONID_CACHE and seeing if that
improves it. Of course, it is probably hard to reproduce, so it would be
tough to know if that helped or not. It would also be nice if you could
dump more information on the error from the ssl library (I typically
build curl against openssl; I wonder if it could be related to using
gnutls or something).

This is OpenSSL, because I also always build against OpenSSL.  :-)

I'll try the CURLOPT_SSL_SESSIONID_CACHE today. It is hard to
reproduce, so not producing it doesn't necessarily mean it isn't still
there.

From: "Shawn O. Pearce" <redacted>

When libcurl fails to connect to an SSL server always retry the
request once. Since the connection failed before the HTTP headers
can be sent, no data has exchanged hands, so the remote side has
not learned of the request and will not perform it twice.

In the wild we have seen git-remote-https fail to connect to
some load-balanced SSL servers sporadically, while modern popular
browsers (e.g. Firefox and Chromium) have no trouble with the same
server pool.

Lets assume the site operators (Hi Google!) have a clue and are
doing everything they already can to ensure secure, successful
SSL connections from a wide range of HTTP clients. Implementing a
single level of retry in the client can make it more robust against
transient failure modes.
---
 http.c        | 19 ++++++++++++-------
 remote-curl.c |  2 ++
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/http.c b/http.c
index 345c171..953f2e6 100644
--- a/http.c
+++ b/http.c

@@ -784,7 +784,7 @@ static int http_request(const char *url, void *result, int target, int options)
 	struct slot_results results;
 	struct curl_slist *headers = NULL;
 	struct strbuf buf = STRBUF_INIT;
-	int ret;
+	int ret, attempts;
 
 	slot = get_active_slot();
 	slot->results = &results;

@@ -820,12 +820,17 @@ static int http_request(const char *url, void *result, int target, int options)
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
 
-	if (start_active_slot(slot)) {
-		run_active_slot(slot);
-		ret = handle_curl_result(slot);
-	} else {
-		error("Unable to start HTTP request for %s", url);
-		ret = HTTP_START_FAILED;
+	for (attempts = 0; attempts < 2; attempts++) {
+		if (start_active_slot(slot)) {
+			run_active_slot(slot);
+			if (slot->results->curl_result == CURLE_SSL_CONNECT_ERROR)
+				continue;
+			ret = handle_curl_result(slot);
+		} else {
+			error("Unable to start HTTP request for %s", url);
+			ret = HTTP_START_FAILED;
+		}
+		break;
 	}
 
 	curl_slist_free_all(headers);

diff --git a/remote-curl.c b/remote-curl.c
index a269608..04a379c 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -353,6 +353,8 @@ static int run_slot(struct active_request_slot *slot)
 
 	slot->results = &results;
 	slot->curl_result = curl_easy_perform(slot->curl);
+	if (slot->curl_result == CURLE_SSL_CONNECT_ERROR)
+		slot->curl_result = curl_easy_perform(slot->curl);
 	finish_active_slot(slot);
 
 	err = handle_curl_result(slot);

-- 
1.7.12.1.590.g4bb1bc4

On Mon, Oct 01, 2012 at 02:23:06PM -0700, Shawn O. Pearce wrote:

From: "Shawn O. Pearce" <redacted>

When libcurl fails to connect to an SSL server always retry the
request once. Since the connection failed before the HTTP headers
can be sent, no data has exchanged hands, so the remote side has
not learned of the request and will not perform it twice.

In the wild we have seen git-remote-https fail to connect to
some load-balanced SSL servers sporadically, while modern popular
browsers (e.g. Firefox and Chromium) have no trouble with the same
server pool.

Lets assume the site operators (Hi Google!) have a clue and are
doing everything they already can to ensure secure, successful
SSL connections from a wide range of HTTP clients. Implementing a
single level of retry in the client can make it more robust against
transient failure modes.

I find this a little distasteful just because we haven't figured out the
actual _reason_ for the failure. That is, I'm not convinced this isn't
something that curl or the ssl library can't handle internally if we
would only configure them correctly. Did you ever follow up on tweaking
the session caching options for curl?

Have you tried running your fails-after-a-few-hours request with other
clients that don't have the problem and seeing what they do (I'm
thinking a small webkit harness or something would be the most
feasible)?

That being said, you did make it so that it only kicks in during ssl
connect errors:

+	for (attempts = 0; attempts < 2; attempts++) {
+		if (start_active_slot(slot)) {
+			run_active_slot(slot);
+			if (slot->results->curl_result == CURLE_SSL_CONNECT_ERROR)
+				continue;
+			ret = handle_curl_result(slot);
+		} else {
+			error("Unable to start HTTP request for %s", url);
+			ret = HTTP_START_FAILED;
+		}
+		break;

which means it shouldn't really be affecting the general populace. So
even though it feels like a dirty hack, at least it is self-contained,
and it does fix a real-world problem. If your answer to the above
questions is "hunting this further is just not worth the effort", I can
live with that.

How come the first hunk gets a nice for-loop and this one doesn't?

Also, are these hunks the only two spots where this error can come up?
The first one does http_request, which handles smart-http GET requests.
the second does run_slot, which handles smart-http POST requests.

Some of the dumb http fetches will go through http_request. But some
will not. And I think almost none of dumb http push will.

-Peff

On Mon, Oct 1, 2012 at 5:23 PM, Shawn O. Pearce [off-list ref] wrote:

From: "Shawn O. Pearce" <redacted>

When libcurl fails to connect to an SSL server always retry the
request once. Since the connection failed before the HTTP headers
can be sent, no data has exchanged hands, so the remote side has
not learned of the request and will not perform it twice.

In the wild we have seen git-remote-https fail to connect to
some load-balanced SSL servers sporadically, while modern popular
browsers (e.g. Firefox and Chromium) have no trouble with the same
server pool.

Lets assume the site operators (Hi Google!) have a clue and are
doing everything they already can to ensure secure, successful
SSL connections from a wide range of HTTP clients. Implementing a
single level of retry in the client can make it more robust against
transient failure modes.

Ok, this begs for some background info...
@Dayjob one of the many things I do is mange our load balancers
(redundant pair in our case). If the attempted SSL connections in one
"bin" (time-slot) exceeds the licensed size of that "bin" then the
excess attempts are just "dropped on the floor." Normal web browsers
detect this initial failure and try again. This may be implemented
internally—I haven't checked.

Google, as I am sure you are well aware, doesn't rely upon a
traditional L2/L3 network level load balancing architecture.
Therefore, I would not attempt to argue that the results that apply to
their systems would apply much of anywhere else. (They have done
presentations publicly, which are archived on the 'net, about how they
do things.)

-- 
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

On Mon, Oct 1, 2012 at 3:18 PM, Jeff King [off-list ref] wrote:

On Mon, Oct 01, 2012 at 02:23:06PM -0700, Shawn O. Pearce wrote:

quoted

When libcurl fails to connect to an SSL server always retry the
request once. Since the connection failed before the HTTP headers
can be sent, no data has exchanged hands, so the remote side has
not learned of the request and will not perform it twice.

I find this a little distasteful just because we haven't figured out the
actual _reason_ for the failure. That is, I'm not convinced this isn't
something that curl or the ssl library can't handle internally if we
would only configure them correctly. Did you ever follow up on tweaking
the session caching options for curl?

No. I didn't try because I reproduced the issue on the initial "GET
/.../info/refs?service=git-upload-pack" request with no authentication
required. So the very first thing the remote-https process did was
fail on an SSL error. During this run I was using a patched Git that
had a different version of the retry logic, but it immediately retried
and the retry was successful. At that point I decided the SSL session
cache wasn't possibly relevant since the first request failed and the
immediate retry was OK.

Have you tried running your fails-after-a-few-hours request with other
clients that don't have the problem and seeing what they do

This is harder to reproduce than you think. It took me about 5 days of
continuous polling to reproduce the error. And I have thus far only
reproduced it against our production servers. This makes it very hard
to test anything. Or to prove that any given patch is better than a
different version.

(I'm
thinking a small webkit harness or something would be the most
feasible)?

So I suspect the contrib/persistent-https proxy thing in Go actually
papers over this problem by having the Go SSL client handle the
connection. But this is only based on a test I ran for several days
through that proxy that did not reproduce the bug. This doesn't mean
it doesn't reproduce with the proxy, it just means _I_ didn't get
lucky with an error in a ~48 hour run.

which means it shouldn't really be affecting the general populace. So
even though it feels like a dirty hack, at least it is self-contained,
and it does fix a real-world problem. If your answer to the above
questions is "hunting this further is just not worth the effort", I can
live with that.

I am sort of at that point, but the hack is so ugly... yea, we
shouldn't have to do this. Or pollute our code with it. I'm willing to
go back and iterate on this further, but its going to be a while
before I can provide any more information.

quoted

diff --git a/remote-curl.c b/remote-curl.c
index a269608..04a379c 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -353,6 +353,8 @@ static int run_slot(struct active_request_slot *slot)

      slot->results = &results;
      slot->curl_result = curl_easy_perform(slot->curl);
+     if (slot->curl_result == CURLE_SSL_CONNECT_ERROR)
+             slot->curl_result = curl_easy_perform(slot->curl);
      finish_active_slot(slot);

How come the first hunk gets a nice for-loop and this one doesn't?

Both hunks retry exactly once after an SSL connect error. I just tried
to pick something reasonably clean to implement. This hunk seemed
simple with the if, the other was uglier and a loop seemed the most
simple way to get a retry in there.

Also, are these hunks the only two spots where this error can come up?
The first one does http_request, which handles smart-http GET requests.
the second does run_slot, which handles smart-http POST requests.

Grrr. I thought I caught all of the curl perform calls but I guess I
missed the dumb transport.

Some of the dumb http fetches will go through http_request. But some
will not. And I think almost none of dumb http push will.

Well, don't use those? :-)

On Mon, Oct 1, 2012 at 10:38 PM, Shawn Pearce [off-list ref] wrote:

On Mon, Oct 1, 2012 at 3:18 PM, Jeff King [off-list ref] wrote:

quoted

On Mon, Oct 01, 2012 at 02:23:06PM -0700, Shawn O. Pearce wrote:

quoted

When libcurl fails to connect to an SSL server always retry the
request once. Since the connection failed before the HTTP headers
can be sent, no data has exchanged hands, so the remote side has
not learned of the request and will not perform it twice.

I find this a little distasteful just because we haven't figured out the
actual _reason_ for the failure.

No. I didn't try because I reproduced the issue on the initial "GET
/.../info/refs?service=git-upload-pack" request with no authentication
required. So the very first thing the remote-https process did was
fail on an SSL error. During this run I was using a patched Git that
had a different version of the retry logic, but it immediately retried
and the retry was successful. At that point I decided the SSL session
cache wasn't possibly relevant since the first request failed and the
immediate retry was OK.

quoted

Have you tried running your fails-after-a-few-hours request with other
clients that don't have the problem and seeing what they do

This is harder to reproduce than you think. It took me about 5 days of
continuous polling to reproduce the error. And I have thus far only
reproduced it against our production servers. This makes it very hard
to test anything. Or to prove that any given patch is better than a
different version.

The only sure way to make sure your patch works is to get your load
balancers Slashdotted first (reason noted in my previous mail on this
subject). For the sake of your relationship with your networking crew
I'd not advise doing that intentionally.

quoted

which means it shouldn't really be affecting the general populace. So
even though it feels like a dirty hack, at least it is self-contained,
and it does fix a real-world problem. If your answer to the above
questions is "hunting this further is just not worth the effort", I can
live with that.

I am sort of at that point, but the hack is so ugly... yea, we
shouldn't have to do this. Or pollute our code with it. I'm willing to
go back and iterate on this further, but its going to be a while
before I can provide any more information.

quoted

How come the first hunk gets a nice for-loop and this one doesn't?

Both hunks retry exactly once after an SSL connect error. I just tried
to pick something reasonably clean to implement. This hunk seemed
simple with the if, the other was uglier and a loop seemed the most
simple way to get a retry in there.

If indeed the problem you are having is with a load balanced setup
then applying TCP/IP like back-off semantics is the right way to go.
The only reason the network stack isn't doing it for you is because
the load balancers wait for the SSL/TLS start before dumping the
"excess" (exceeding of license) SSL connections.

-- 
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59