I didn't see anything named ShortCircuitSharedMemorySegment in the patch, should it be included?
It should be there...
Javadoc for SharedFileDescriptorFactory constructor
rand() isn't reentrant, potentially making it unsafe for createDescriptor0. Should we use rand_r instead, or slap a synchronized on it?
Apparently, on Linux rand is re-entrant because glibc puts a mutex around it. But you're right, we should be POSIX-compliant here. I added a mutex around rand. Using the reentrant versions would be awkward because of the need to pass around state somehow (probably a java array).
Also not sure why we concat two rand(). Seems like one should be enough with the collision detection code.
The open is done with mode 0777, wouldn't 0700 be safer? I thought we were passing these over a domain socket, so we can keep the permissions locked up.
Good point. we don't want random users to be able to open this file during the brief period it exists in the namespace.
Paranoia, should we do a check in CloseableReferenceCount#reference for overflow to the closed bit? I know we have 30 bits, but who knows.
Well, this code was just moved from DomainSocket.java, not changed.
The issue is that we want to use atomic addition, not compare-and-exchange, for speed. Given that, all we know is the state after the addition, not before. This is fairly performance-critical for UNIX domain sockets (it has to do this before every socket operation) so it has to be fast. The failure mode also seems fairly benign: the refcount overflows into the closed bit and causes the socket to appear closed.
At some point we should evaulate a 64-bit counter. It might be just as fast on 64-bit machines.
Unrelated nit: DomainSocket#write(byte, int, int) boolean exec is indented wrong, mind fixing it?
[DomainSocketWatcher] javadoc is c+p from DomainSocket, I think it should be updated for DSW. Some high-level description of how the nested classes fit together would be nice.
Some Java-isms. Runnable is preferred over Thread. It's also weird that DSW is a Thread subclass and it calls start on itself. An inner class implementing Runnable would be more idiomatic.
It's kind of annoying that using an inner Runnable class would increase the indentation of run(). Still, I suppose it does provide better isolation, making it impossible to invoke random Thread methods on the DomainSocketWatcher. So I will implement that.
Explain use of loopSocks 0 versus loopSocks 1? This is a crucial part of this class: we need to use a socketpair rather than a plain condition variable because of blocking on poll.
It's arbitrary: both sockets are connected to one another and exactly alike. I chose to listen on 1 and write on 0, but I could easily have made the opposite choice.
"loopSocks" is also not a very descriptive name, maybe "wakeupPair" or "eventPair" instead?
I changed it to notificationSockets.
Can add a Precondition check to make sure the lock is held in checkNotClosed
If we fail to kick, add and remove could block until the poll timeout.
Should doc that we only support one Handler per fd, it overwrites on add. Maybe Precondition this instead if we don't want to overwrite, I can't tell from context here.
The repeated calls to sendCallback are worrisome. For instance, a sock could be EOF and closed, be removed by the first sendCallback, and then if there's a pending toRemove for the sock, the second sendCallback aborts on the Precondition check.
Good catch. Fixed.
closeAll parameter in sendCallback is unused
This comment probably means to refer to loopSocks: // Close shutdownSocketPair, so that shutdownSocketPair gets an EOF
This comment probably meant poll, not select: // were waiting in select().
Why are two of the @Test in TestDomainSocketWatcher commented out?
Timeouts seem kind of long, these should be super fast tests right?
reduced. I didn't want to reduce too much to avoid flakiness.