On 18/10/2024 06:29, Jeff King wrote: > On Fri, Oct 18, 2024 at 06:36:11AM +0200, Patrick Steinhardt wrote: > >> Subject: builtin/credential-cache--daemon: fix error when "exit"ing on Cygwin > > I think this commit message has a few unclear or inaccurate bits, > because it's based on the earlier attempt. E.g., the change is now on > the client side, not in credential-cache--daemon. > > And I think rather than say "the daemon exit rules are intricate", we > can actually outline the rules. :) > > So here's what I had written after reading through the old thread. It > would preferably get Ramsay's Signed-off-by before being applied. Oh Wow, this is (no surprise) a masterpiece! :) I would very happily add: Signed-off-by: Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxxx> (I don't think I could have produced anything half as good in several weekends!) Thanks! ATB, Ramsay Jones > > -- >8 -- > From: Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxxx> > Subject: [PATCH] credential-cache: treat ECONNABORTED like ECONNRESET > > On Cygwin, t0301 fails because "git credential-cache exit" returns a > non-zero exit code. What's supposed to happen here is: > > 1. The client (the "credential-cache" invocation above) connects to a > previously-spawned credential-cache--daemon. > > 2. The client sends an "exit" command to the daemon. > > 3. The daemon unlinks the socket and then exits, closing the > descriptor back to the client. > > 4. The client sees EOF on the descriptor and exits successfully. > > That works on most platforms, and even _used_ to work on Cygwin. But > that changed in Cygwin's ef95c03522 (Cygwin: select: Fix FD_CLOSE > handling, 2021-04-06). After that commit, the client sees a read error > with errno set to ECONNABORTED, and it reports the error and dies. > > It's not entirely clear if this is a Cygwin bug. It seems that calling > fclose() on the filehandles pointing to the sockets is sufficient to > avoid this error return, even though exiting should in general look the > same from the client's perspective. > > However, we can't just call fclose() here. It's important in step 3 > above to unlink the socket before closing the descriptor to avoid the > race mentioned by 7d5e9c9849 (credential-cache--daemon: clarify "exit" > action semantics, 2016-03-18). The client will exit as soon as it sees > the descriptor close, and the daemon may or may not have actually > unlinked the socket by then. That makes test code like this: > > git credential exit && > test_path_is_missing .git-credential-cache > > racy. > > So we probably _could_ fix this by calling: > > delete_tempfile(&socket_file); > fclose(in); > fclose(out); > > before we exit(). Or by replacing the exit() with a return up the stack, > in which case the fclose() happens as we unwind. But in that case we'd > still need to call delete_tempfile() here to avoid the race. > > But simpler still is that we can notice that we already special-case > ECONNRESET on the client side, courtesy of 1f180e5eb9 (credential-cache: > interpret an ECONNRESET as an EOF, 2017-07-27). We can just do the same > thing here (I suspect that prior to the Cygwin commit that introduced > this problem, we were really just seeing ECONNRESET instead of > ECONNABORTED, so the "new" problem is just the switch of the errno > values). > > There's loads more debugging in this thread: > > https://lore.kernel.org/git/9dc3e85f-a532-6cff-de11-1dfb2e4bc6b6@xxxxxxxxxxxxxxxxxxxx/ > > but I've tried to summarize the useful bits in this commit message. > > [jk: commit message] > > Signed-off-by: Jeff King <peff@xxxxxxxx> > --- > 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 5de8b9123b..7789d57d3e 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)
