On 07/07/2022 19:12, Jeff King wrote:

On Thu, Jul 07, 2022 at 02:50:21AM +0100, Ramsay Jones wrote:

quoted

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/ (local)

I have now read (much) of that thread and it makes sense now. (Yes, I probably
should have postponed sending the email until after researching some more today;
lesson learned).

[snip]

quoted

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).

Yes, I was going to ask about ECONNRESET ... heh, no I'm kidding! :)

Yeah, if we can't determine the reason for cygwin changing behaviour
here (and fix it in cygwin), then this is probably the simplest solution.

ATB,
Ramsay Jones