fix http deadlock on giant ref negotiations
Get series with b4
b4 am [email protected]Versions newest first · linked by author and title
- v2 Latest3 patches 3 repliesIn review
- v12 patches 8 repliesSuperseded
Patches in v2 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/3 | http-backend: fix die recursion with custom handlerhttp-backend.c | +9 | 0 replies | |
| 2/3 | t5551: factor out tag creationt/ | +21 | 0 replies | |
| 3/3 | http-backend: spool ref negotiation requests to bufferDocumentation/ http-backend.c t/ | +110 | 3 replies |
Reviews trailers per patch
No Reviewed-by, Acked-by or Tested-by trailers for v2 yet.
Cover letter [PATCH v2 0/3] fix http deadlock on giant ref negotiations
Quoted 11 lines · Konstantin Ryabitsev
Quoted 4 lines · Junio C Hamano
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).
Quoted 7 lines
E.g. can be set via an Apache directive like
SetEnv
FOO_MAX_SIZE2048The backend can then be configured to emit an error message when the spool size is exhausted saying "foo exhausted, increase
FOO_MAX_SIZEto 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:
t/t5551-http-fetch-smart.sh +10 -0
@@ -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