On 2/9/21 11:32 AM, Jeff King wrote:
On Fri, Feb 05, 2021 at 06:28:13PM -0500, Jeff Hostetler wrote:
OK. I'm still not sure of the endgame here for writing non-racy code to
establish the socket (which is going to require either some atomic
renaming or some dot-locking in the caller). But it's plausible to me
that this option will be a useful primitive.
In part 14/14 in `ipc-unix-sockets.c:create_listener_socket()` I have
code in the calling layer to (try to) handle both the startup races
and basic collisions with existing long-running servers already using
the socket.
There you make a temp socket and then try to rename it into place. But
because rename() overwrites the destination, it still seems like two
creating processes can race each other. Something like:
0. There's no "foo" socket (or maybe there is a stale one that
nobody's listening on).
1. Process A wants to become the listener. So it creates foo.A.
2. Process B likewise. It creates foo.B.
3. Process A renames foo.A to foo. It believes it will now service
clients.
4. Process B renames foo.B to foo. Now process A is stranded but
doesn't realize it.
Yeah, in my version two processes could still create uniquely named
sockets and then do the rename trick. But they capture the inode
number of the socket before they do that. They periodically lstat
the socket to see if the inode number has changed and if so, assume
it has been stolen from them. (A bit of a hack, I admit.)
And I was assuming that 2 servers starting at about the same time
are effectively equivalent -- it doesn't matter which one dies, since
they both should have the same amount of cached state. Unlike the
case where a long-running server (with lots of state) is replaced by
a newcomer.
I.e., I don't think this is much different than an unlink+create
strategy. You've eliminated the window where a process C shows up during
steps 3 and 4 and sees no socket (because somebody else is in the midst
of a non-atomic unlink+create operation). But there's no atomicity
between the "ping the socket" and "create the socket" steps.
But you're right, it might be good to revisit that as a primitive at
this layer. We only have 1 other caller right now and I don't know
enough about `credential-cache--daemon` to know if it would benefit
from this or not.
Yeah, having seen patch 14, it looks like your only new caller always
sets the new unlink option to 1. So it might not be worth making it
optional if you don't need it (especially because the rename trick,
assuming it's portable, is superior to unlink+create; and you'd always
be fine with an unlink on the temp socket).
I am wondering if we can use the .LOCK file magic to our advantage
here (in sort of an off-label use). If we have the server create a
lockfile "<path>.LOCK" and if successful leave it open/locked for the
life of the server (rather than immediately renaming it onto <path>)
and let the normal shutdown code rollback/delete the lockfile in the
cleanup/atexit.
If the server successfully creates the lockfile, then unlink+create
the socket at <path>.
That would give us the unique/exclusive creation (on the lock) that
we need. Then wrap that with all the edge case cleanup code to
create/delete/manage the peer socket. Basically if the lock exists,
there should be a live server listening to the socket (unless there
was a crash...).
And yes, then I don't think I need the `preserve_existing` bit in the
opts struct.
The call in credential-cache--daemon is definitely racy. It's pretty
much the same thing: it pings the socket to see if it's alive, but is
still susceptible to the problem above. I was was never too concerned
about it, since the whole point of the daemon is to hang around until
its contents expire. If it loses the race and nobody contacts it, the
worst case is it waits 30 seconds for somebody to give it data before
exiting. It would benefit slightly from switching to the rename
strategy, but the bigger race would remain.
-Peff
