[PATCH v2 0/3] fix http deadlock on giant ref negotiations

[Jeff King <peff@peff.net>]

On Fri, May 15, 2015 at 02:28:37PM -0400, Konstantin Ryabitsev wrote:

> On 15/05/15 02:22 PM, Junio C Hamano wrote:
> > Also, is it worth allocating small and then growing up to the maximum?
> > I think this only relays one request at a time anyway, and I suspect
> > that a single 1MB allocation at the first call kept getting reused
> > may be sufficient (and much simpler).
> 
> Does it make sense to make that configurable via an env variable at all?
> I suspect the vast majority of people would not hit this bug unless they
> are dealing with repositories polluted with hundreds of refs created by
> automation (like the codeaurora chromium repo).
> 
> E.g. can be set via an Apache directive like
> 
> SetEnv FOO_MAX_SIZE 2048
> 
> The backend can then be configured to emit an error message when the
> spool size is exhausted saying "foo exhausted, increase FOO_MAX_SIZE to
> allow for moar foo."

Yeah, that was the same conclusion I came to elsewhere in the thread.
Here's a re-roll:

  [1/3]: http-backend: fix die recursion with custom handler
  [2/3]: t5551: factor out tag creation
  [3/3]: http-backend: spool ref negotiation requests to buffer

It makes the size configurable (either through the environment, which is
convenient for setting via Apache; or through the config, which is
convenient if you have one absurdly-sized repo). It mentions the
variable name when it barfs into the Apache log. I also bumped the
default to 10MB, which I think should be enough to handle even
ridiculous cases.

I also adapted Dennis's test into the third patch. Beware that it's
quite slow to run (and is protected by the "EXPENSIVE" prerequisite).
Patch 2 is new, and just refactors the script to make adding the new
test easier.

I really wanted to add a test like:

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index e2a2fa1..1fff812 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -273,6 +273,16 @@ test_expect_success 'large fetch-pack requests can be split across POSTs' '
 	test_line_count = 2 posts
 '
 
+test_expect_success 'http-backend does not buffer arbitrarily large requests' '
+	test_when_finished "(
+		cd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&
+		test_unconfig http.maxrequestbuffer
+	)" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
+		config http.maxrequestbuffer 100 &&
+	test_must_fail git clone $HTTPD_URL/smart/repo.git foo.git
+'
+
 test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
 	(
 		cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&

to test that the maxRequestBuffer code does indeed work. Unfortunately,
even though the server behaves reasonably in this case, the client ends
up hanging forever. I'm not sure there is a simple solution to that; I
think it is a protocol issue where remote-http is waiting for fetch-pack
to speak, but fetch-pack is waiting for more data from the remote.

Personally, I think I'd be much more interested in pursuing a saner,
full duplex http solution like git-over-websockets than trying to iron
out all of the corner cases in the stateless-rpc protocol.

-Peff