As I mention in my other answers (about protocol v2 and pack files), the addition of the Git Wire Protocol, Version 2 changes how the Git's transfer protocol works.
And it is the default one since Git 2.26.
In particular, git fetch will be faster, because of a recent change:
With Git 2.33 (Q3 2021), "git fetch"(man) over protocol v2 left its side of the socket open after it finished speaking, which unnecessarily wasted the resource on the other side.
See commit ae1a7ee (19 May 2021) by Jeff King (peff).
(Merged by Junio C Hamano -- gitster -- in commit 4dd75a1, 14 Jun 2021)
fetch-pack: signal v2 server that we are done making requests
Reported-by: Greg Pflaum
Signed-off-by: Jeff King
When fetching with the v0 protocol over ssh (or a local upload-pack with pipes), the server closes the connection as soon as it is finished sending the pack.
So even though the client may still be operating on the data via index-pack (e.g., resolving deltas, checking connectivity, etc), the server has released all resources.
With the v2 protocol, however, the server considers the ssh session only as a transport, with individual requests coming over it.
After sending the pack, it goes back to its main loop, waiting for another request to come from the client.
As a result, the ssh session hangs around until the client process ends, which may be much later (because resolving deltas, etc, may consume a lot of CPU).
This is bad for two reasons:
- it's consuming resources on the server to leave open a connection that won't see any more use
- if something bad happens to the ssh connection in the meantime (say, it gets killed by the network because it's idle, as happened in a real-world report), then ssh will exit non-zero, and we'll propagate the error up the stack.
The server is correct here not to hang up after serving the pack.
The v2 protocol's design is meant to allow multiple requests like this, and hanging up would be the wrong thing for a hypothetical client which was planning to make more requests (though in practice, the git.git client never would, and I doubt any other implementations would either).
The right thing is instead for the client to signal to the server that it's not interested in making more requests.
We can do that by closing the pipe descriptor we use to write to ssh.
This will propagate to the server upload-pack as an EOF when it tries to read the next request (and then it will close its half, and the whole connection will go away).
It's important to do this "half duplex" shutdown, because we have to do it before we actually receive the pack.
This is an artifact of the way fetch-pack and index-pack (or unpack-objects) interact.
We hand the connection off to index-pack (really, a sideband demuxer which feeds it), and then wait until it returns.
And it doesn't do that until it has resolved all of the deltas in the pack, even though it was done reading from the server long before.
So just closing the connection fully after index-pack returns would be too late; we'd have held it open much longer than was necessary.
And teaching index-pack to close the connection is awkward.
It's not even seeing the whole conversation (the sideband demuxer is, but it doesn't actually know what's in the packets, or when the end comes).
Note that this close() is happening deep within the transport code.
It's possible that a caller would want to perform other operations over the same ssh transport after receiving the pack.
But as of the current code, none of the callers do, and there haven't been discussions of any plans to change this.
If we need to support that later, we can probably do so by passing down a flag for "you're the last request on the transport; it's OK to close" instead of the code just assuming that's true.
The description above all discusses v2 ssh, so it's worth thinking about how this interacts with other protocols:
- in v0 protocols, we could do the same half-duplex shutdown (it just goes into the v0
do_fetch_pack() instead).
This does work, but since it doesn't have the same persistence problem in the first place, there's little reason to change it at this point.
- local fetches against
git-upload-pack(man) on the same machine will behave the same as ssh (they are talking over two pipes, and see EOF on their input pipe)
- fetches against
git-daemon(man) will run this same code, and close one of the descriptors.
In practice, this won't do anything, since there our two descriptors are dups of each other, and not part of a half-duplex pair.
The right thing would probably be to call shutdown(SHUT_WR) on it.
I didn't bother with that here.
It doesn't face the same error-code problem (since it's just a TCP connection), so it's really only an optimization problem.
And git:// is not that widely used these days, and has less impact on server resources than an ssh termination.
- v2 http doesn't suffer from this problem in the first place, as our pipes terminate at a local
git-remote-https, which is passing data along as individual requests via curl.
Probably curl is keeping the TCP/TLS connection open for more requests, and we might be able to tell it manually "hey, we are done making requests now".
But I think that's much less important.
It again doesn't suffer from the error-code problem, and HTTP keepalive is pretty well understood (importantly, the timeouts can be set low, because clients like curl know how to reconnect for subsequent requests if necessary).
So it's probably not worth figuring out how to tell curl that we're done (though if we do, this patch is probably the first step anyway; fetch-pack closes the pipe back to remote-https, which would be the signal that it should tell curl we're done).
The code is pretty straightforward.
We close the pipe at the right moment, and set it to -1 to mark it as invalid.
I modified the later cleanup code to avoid calling close(-1).
That's not strictly necessary, since close(-1) is a noop, but hopefully makes things a bit more obvious to a reader.
I suspect that trying to call more transport functions after the close() (e.g., calling transport_fetch_refs() again) would fail, as it's not smart enough to realize we need to re-open the ssh connection.
But that's already true when v0 is in use.
And no current callers want to do that (and again, the solution is probably a flag in the transport code to keep things open, which can be added later).
