Clients can signal the git-credential-cache(1) daemon that it is
supposed to exit by sending it an "exit" command. The details around
how exactly the daemon exits seem to be rather intricate as spelt out by
a comment surrounding our call to exit(3p), as we need to be mindful
around closing the client streams before we signal the client.
The logic is broken on Cygwin though: when a client asks the daemon to
exit, they won't see the EOF and will instead get an error message:
fatal: read error from cache daemon: Software caused connection abort
This issue is known in Cygwin for quite a while already [1] [2] and
seems to come from a change in Cygwin itself. While we haven't figured
out what exact change that was, the effect is that we see ECONNABORTED
now instead of ECONNRESET when trying to read from the socket connected
to the daemon when the daemon has shut down. It is somewhat dubious that
errno changed in this way: read(3p) does not mention ECONNABORTED as a
possible error code, so the behaviour seems to not be POSIX-compliant.
We have discussed a couple of workarounds:
- Explicitly close file streams when handling the "exit" action in
`serve_one_client()`. This may easily lead to races because we need
to be very careful about the order in which we delete sockets.
- We can adapt `serve_one_client()` to not use exit(3) anymore, and
instead handle the shutdown in the outer loop. This allows us to
handle the shutdown elsewhere and be less intimate with the calling
context.
- Start handling ECONNABORTED.
The last option seems like it is both the easiest and least-risky fix.
It is unlikely that it would negatively affect any other platforms given
that read(3p) shouldn't even set the code in the first place, and it
fixes the issue on Cygwin.
Fix the issue by handling both ECONNRESET and ECONNABORTED.
[1]: https://github.com/cygporter/git/issues/51
[2]: https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@xxxxxxxxxxxxxxxxxxxx/
Original-patch-by: Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxxx>
Helped-by: Jeff King <peff@xxxxxxxx>
Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
---
builtin/credential-cache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c
index 5de8b9123bf..7789d57d3e1 100644
--- a/builtin/credential-cache.c
+++ b/builtin/credential-cache.c
@@ -30,7 +30,7 @@ static int connection_fatally_broken(int error)
static int connection_closed(int error)
{
- return (error == ECONNRESET);
+ return error == ECONNRESET || error == ECONNABORTED;
}
static int connection_fatally_broken(int error)
base-commit: a7c6fbb48a5849db1bb1f841ef5403476fed0c0e
--
2.47.0.72.gef8ce8f3d4.dirty
