Re: t0301-credential-cache test failure on cygwin

> On Thu, Jul 07, 2022 at 02:50:21AM +0100, Ramsay Jones wrote:
> 
>> Having deleted the above patch, I now had a look at the server side. Tracing
>> out the server execution showed no surprises - everything progressed as one
>> would expect and it 'exit(0)'-ed correctly! The relevant part of the code to
>> process a client request (in the serve_one_client() function, lines 132-142
>> in builtin/credential-cache--daemon.c) looks like:
>>
>> 	else if (!strcmp(action.buf, "exit")) {
>> 		/*
>> 		 * It's important that we clean up our socket first, and then
>> 		 * signal the client only once we have finished the cleanup.
>> 		 * Calling exit() directly does this, because we clean up in
>> 		 * our atexit() handler, and then signal the client when our
>> 		 * process actually ends, which closes the socket and gives
>> 		 * them EOF.
>> 		 */
>> 		exit(0);
>> 	}
>>
>> Now, the comment doesn't make clear to me why "it's important that we clean
>> up our socket first" and, indeed, whether 'socket' refers to the socket
>> descriptor or the socket file. In the past, all of my unix-stream-socket
>> servers have closed the socket descriptor and then unlink()-ed the socket
>> file before exit(), with no 'atexit' calls in sight (lightly cribbed from a
>> 30+ years old Unix Network programming book by Stevens - or was it the Comer
>> book - or maybe the Comer and Stevens book - I forget!).
> 
> That comment refers to the socket file. If we close the handle to the
> client before we clean up the socket file, then the client may finish
> while the socket file is still there. So anybody expecting that:
> 
>   git credential-cache exit
> 
> is a sequencing operation will be fooled. One obvious thing is:
> 
>   git credential-cache exit
>   git credential-cache store <some-cred
> 
> which is now racy; the second command may try to contact the socket for
> the exiting daemon. It might actually handle that gracefully (because
> the server wouldn't actually accept()) but I didn't check. But another
> example, and the one that motivated that comment is:
> 
>   git credential-cache exit
>   test_path_is_missing $HOME/.git-credential-cache/socket
> 
> which is exactly what our tests do. ;) See the discussion around here:
> 
>   https://lore.kernel.org/git/20160318061201.GA28102@sigill.intra.peff.net/

>> So, we now have three patches which 'fix' the issue. What does this tell us?
>> Well, not an awful lot! ;-)
> 
> Of the three, I actually like the client-side one to check errno the
> best. The client is mostly "best effort". If it can't talk to the daemon
> for whatever reason, then it becomes a noop (there is nothing it can
> retrieve from the cache, and if it's trying to write, then oh well, the
> cached value was immediately expired!).
> 
> So one could argue that _every_ read error should be silently ignored.
> Calling die_errno() is mostly a nicety for debugging a broken setup, but
> in normal use, the outcome is the same either way (and Git will
> certainly ignore the exit code credential-cache anyway). I prefer the
> "ignore known harmless errors" approach, possibly because I am often the
> one debugging. ;) If ECONNABORTED is a harmless error we see in
> practice, I don't mind adding it to the list (under the same rationale
> as the current ECONNRESET that is there).