-
Notifications
You must be signed in to change notification settings - Fork 5k
Implement NamedPipe*Stream on Unix domain sockets #6833
Implement NamedPipe*Stream on Unix domain sockets #6833
Conversation
struct ucred creds; | ||
socklen_t len = sizeof(creds); | ||
return getsockopt(fd, SOL_SOCKET, SO_PEERCRED, &creds, &len) == 0 ? | ||
GetNameFromUid(creds.uid) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a side-effect of the condition here feels odd to me. I think I'd rather this just be an if/else statement, so the execution order is more apparent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? I rather like this construction 😄 If it really bothers you, I can change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't insist. :)
@stephentoub One thing I thought weird about pipe streams was that |
|
||
return (long)handle >= 0 ? | ||
Interop.Sys.Close(handle) == 0 : | ||
true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would we end up with a handle that needs to be closed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anonymous pipes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh. Might be good to have a comment to that effect.
It needs to be based on the contract provided by Stream.Write/WriteAsync. |
@stephentoub I see. Is there a plan to provide an addition to the contract that allows for partial writes? |
I'm not clear on what behavior you're hoping for. You want a write method that waits until it can write at least 1 byte and then writes as much as it can and returns an Int32 for how much it wrote? There are no plans for such a thing. |
@stephentoub Yeah, I edited the comment -- that's what I was asking about. You answered the question though, thanks! |
|
||
struct passwd pw; | ||
struct passwd* result; | ||
if (getpwuid_r(uid, &pw, buffer, bufferLength, &result) == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result
value is never freed. Is that intentional. Could not find a reference to whether this needs to be freed or not in the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit of a strangely designed function. result is set to &pw on success or to NULL if the entry couldn't be found. Either way, nothing to be freed.
A couple of nits, otherwise LGTM. |
4f5dad8
to
dea6316
Compare
dea6316
to
7dea5fa
Compare
@dotnet-bot test this please |
Unix domain sockets can result in ENOENT failures due to the specified file path not existing. There's no corresponding entry in SocketError, so it often ends up showing up as an unknown -1 error. This commit just adds a mapping to the closest entry possible, AddressNotAvailable, making it both a bit easier to diagnose and making it possible to filter based on AddressNotAvailable in consuming code.
A previous change allowed AddressFamily.Unix and non-IPEndPoints to be used in System.Net.Sockets, removing some asserts and changing some if/elses to support it. One case was missed, though, in ConnectAsync, which causes asserts and runtime failures when using Socket.ConnectAsync with Unix domain sockets. This commit just makes the same change to that code path that was made to other similar ones.
Today, NamedPipeServer/ClientStream are implemented on top of FIFOs, aka named pipes. There are some unfortunate limitations to this that are causing problems for typical usage of NamedPipeServer/ClientStream: - FIFOs can be constructed as both read and write (i.e. InOut), but a writer can then immediately read what was written. In other words, whereas one might expect there two be two data channels, one in each direction, there's only one that everyone can read and write from. - FIFOs can be connected to by any number of readers and writers, which has the effect of meaning a reader doesn't unblock when a writer disconnects, because there may be another writer in the future. To address these significant limitations, this commit moves NamedPipeServer/ClientStream to be built instead on top of Unix domain sockets. Unix domain sockets are an IPC mechanism for local communication using the sockets model, and we have support for it via System.Net.Sockets. Both of the aforementioned problems are addressed by this change. A few other positive outcomes: - The participants on each side of the connection can get the credentials of the other side, so GetImpersonatedUserName is now implemented. - Our Socket implementation has a good async implementation, so we can use it for all of the async operations exposed rather than queueing a work item that then blocks doing the synchronous operation. There are a few downsides, but they're worth the tradeoff: - FIFOs have the same general blocking behavior as anonymous pipes and as named pipes on Windows: a call to Write doesn't complete until an associated Read comes along. That's not the case with Unix domain sockets, where a Write typically just deposits the data into a buffer. Writes on a socket only block when the buffer is full. - Unix domain sockets are considered to be a bit slower than FIFOs. - Changing the buffer size of a FIFO impacts the whole FIFO, whereas changing the buffer size for a socket impacts just that end of the socket. There continue to be several features that we don't support, e.g. message transmission, max number of servers, etc.
Also add/update a few tests.
7dea5fa
to
c39ac9a
Compare
Implement NamedPipe*Stream on Unix domain sockets
@stephentoub why is UnixDomainSocketEndPoint no longer public? |
It was just sample code that was only compiled into a test; it wasn't actually public in a shipping assembly nor in a contract. |
@stephentoub is the intention to make it public? are there missing bits? |
Not for an initial release. Maybe at some point in the future. In the meantime, if you need them, you can copy the end point implementation into your project. |
Is there any intention to expose an equivalent to the |
…domainsockets Implement NamedPipe*Stream on Unix domain sockets Commit migrated from dotnet/corefx@3ebf2fc
Today, NamedPipeServer/ClientStream on Unix are implemented on top of FIFOs, aka named pipes. There are some unfortunate limitations to this that are causing problems for typical usage of NamedPipeServer/ClientStream:
To address these significant limitations, this commit moves NamedPipeServer/ClientStream to be built instead on top of Unix domain sockets. Unix domain sockets are an IPC mechanism for local communication using the sockets model, and we have support for it via System.Net.Sockets, thanks to #5051.
Both of the aforementioned problems are addressed by this change. A few other positive outcomes:
There are a few downsides, but they're worth the tradeoff:
There continue to be several features that we don't support on Unix, e.g. message transmission, max number of servers, etc.
(One oddity here is that this makes the Unix implementation of System.IO.Pipes.dll have a dependency on System.Net.Sockets.dll, but I spoke with @weshaggard and, other than it being odd, we couldn't come up with any strong reason not to. In the layering, Sockets is already considered lower-level.)
cc: @ericeil, @ianhays, @bartonjs, @jaredpar, @AArnott, @CIPop, @ericstj, @dsplaisted, @IlyaBiryukov, @bpschoch
Fixes https://github.com/dotnet/corefx/issues/1849