On Tue, Apr 25, 2017 at 05:39:16PM -0600, Daniel Wallace wrote:
> I am not sure if this is a regression or not, but I wanted to get feedback.
>
> It looks like this commit changed some behavior in git-http-backend
>
> https://git.kernel.org/pub/scm/git/git.git/commit/?id=6bc0cb5176a5e42ca4a74e3558e8f0790ed09bb1
>
> The change that it has made is that it no git-upload-pack hangs when
> uwsgi doesn't close stdin.
Yeah, I think that could be considered an unintended regression. The
original code _also_ didn't respect CONTENT_LENGTH and would potentially
just keep reading. But because it was reading formatted data, as long as
the data was well-formed, it would generally stop reading where the
webserver expected it. So it mostly worked; even though there were cases
that would hang, they were presumably rare.
People using IIS ran into a similar thing, I think. There was a patch,
but it had a lot of problems. I do think it would be reasonable to:
1. Respect CONTENT_LENGTH if it's set, and never read more than that
many bytes.
2. Handle sentinel values for CONTENT_LENGTH that tell us the data is
streaming in and we must read until EOF (Apache leaves
CONTENT_LENGTH unset in this case, and apparently IIS sets it to
-1).
3. Ideally, do both of those not just in the buffering case added by
6bc0cb517, but for other input as well. That might be hard, though,
because I think we literally hand off the descriptor to
sub-programs for some operations. So even if we just handled the
one case, that would at least fix any regressions caused by
6bc0cb517.
Here are links to the previous discussion:
http://public-inbox.org/git/f0f5a56a22f20d4cb4a03bb8d6658797e260e...@server2011.cs-SOFTWARE.local/
http://public-inbox.org/git/f0f5a56a22f20d4cb4a03bb8d6658797e261a...@server2011.cs-SOFTWARE.local/
-Peff
