| Message ID | 20241018052952.GE2408674@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 468a7e41e87eb95d27563d111a36ddca0822e5f6 |
| Headers | show |
| Series | credential-cache: treat ECONNABORTED like ECONNRESET | expand |
On Fri, Oct 18, 2024 at 01:29:52AM -0400, 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. Works for me. I don't really care whose patch lands, I just want the issue to be fixed :) Thanks! Patrick
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@ramsayjones.plus.com> (I don't think I could have produced anything half as good in several weekends!) Thanks! ATB, Ramsay Jones > > -- >8 -- > From: Ramsay Jones <ramsay@ramsayjones.plus.com> > 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@ramsayjones.plus.com/ > > but I've tried to summarize the useful bits in this commit message. > > [jk: commit message] > > Signed-off-by: Jeff King <peff@peff.net> > --- > 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)
On Fri, Oct 18, 2024 at 04:33:13PM +0100, Ramsay Jones wrote: > > > 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@ramsayjones.plus.com> Thanks, all. Thanks, Taylor
On Fri, Oct 18, 2024, at 07:29, Jeff King wrote: > [jk: commit message] > > Signed-off-by: Jeff King <peff@peff.net> Just curious. Why this bracket comment (non-trailer line or whatever it is called) and not a regular trailer? All the bracket comments that I’ve seen give some comment, often about tweaking the patch or the commit message. In this case though the whole commit message is written by one person.
On Fri, Oct 18, 2024 at 11:27:50PM +0200, Kristoffer Haugsbakk wrote: > On Fri, Oct 18, 2024, at 07:29, Jeff King wrote: > > [jk: commit message] > > > > Signed-off-by: Jeff King <peff@peff.net> > > Just curious. Why this bracket comment (non-trailer line or whatever it > is called) and not a regular trailer? All the bracket comments that > I’ve seen give some comment, often about tweaking the patch or the > commit message. In this case though the whole commit message is written > by one person. I assigned authorship to Ramsay, so my name is not otherwise mentioned, but appears in the signoff. So it was a way of mentioning what I contributed (both for credit, but also in case anybody has questions later). I guess "Commit-message-by:" would work, too. ;) I guess you could likewise argue that I'm a co-author. I think in the usual trailer order, it would be: Signed-off-by: Ramsay [jk: add commit message] Signed-off-by: me but I didn't want to forge his S-o-b without asking first. -Peff
Hi Peff (editing the subject now which I intended to do from the start) On Sat, Oct 19, 2024, at 23:21, Jeff King wrote: >> On Fri, Oct 18, 2024, at 07:29, Jeff King wrote: >> […] > > I assigned authorship to Ramsay, so my name is not otherwise mentioned, > but appears in the signoff. So it was a way of mentioning what I > contributed (both for credit, but also in case anybody has questions > later). > > I guess "Commit-message-by:" would work, too. ;) I’ve done that when someone has given me a non-descript diff. :) > I think in the usual trailer order, it would be: > > Signed-off-by: Ramsay > [jk: add commit message] > Signed-off-by: me > > but I didn't want to forge his S-o-b without asking first. I’ve seen those brackets in the log. They used to happen with some regularity. At first it made sense since you need a free-form area to both comment and tell everyone that you left the comment. And a trailer doesn’t make sense for that, I thought.[1] But thinking about the signoff requirement: you already have all the information you need from the next trailer, namely the signoff. In other words this: [kh: Added tests] Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> Has the same information as this: Comment: Added tests Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> Because the signoff order tells you who left the comment. So I was wondering to myself why this uniform approach wasn’t used. † 1: Since the brackets become “non-trailer values” or something (git-interpret-trailers(1)), i.e. the discarded parts of the trailer block
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)