Use new CoreFX API for creating NamedPipeServerStreams#34607
Use new CoreFX API for creating NamedPipeServerStreams#34607agocke merged 2 commits intodotnet:masterfrom
Conversation
|
test mac please |
The default behavior for the NamedPipeServerStream API is to take a pipe name and then construct a named pipe in the background using that name. In Windows this involves creating a file in a special namespace in the file system. On Unix, named pipes are implemented using Unix Domain Sockets, which are actual files, and the CoreFX behavior is to create them in the temporary directory. Unfortunately, Unix Domain Sockets also often have a max path length limitation and the temporary directory could be arbitrarily long, meaning that any attempt to create a named pipe may fail on Unix. To remedy this, CoreFX introduced an API which allows you to pass a full path instead of just a pipe name. If a fully-qualifed path is passed, the new behavior is used. We can use this functionality to improve reliability of pipe name creation by using the "/tmp" directory on Unix, which by the POSIX specification is always required to be a valid temporary directory, and by using a fixed-length pipe name that is lower than any known Unix Domain Socket path length restriction. This should fix the issue for good. Fixes dotnet#24137
| _tempDirectory, | ||
| s_helloWorldSrcCs, | ||
| shouldRunOnServer: false); | ||
| shouldRunOnServer: true); |
There was a problem hiding this comment.
Can you explain the false->true and 0->1 changes here?
There was a problem hiding this comment.
The test previously tested that the server failed. So the server would start, then shutdown, and the command line compiler would fall back to in-memory compilation. So we previously asserted that the request would not run on the server, and that the server exited without processing any compilations.
Now we do the opposite (because I fixed it) -- the compilation runs on the server successfully, and the server processes one compilation.
| /// </summary> | ||
| /// <returns></returns> | ||
| [ConditionalFact(typeof(WindowsOrLinuxOnly))] | ||
| [Fact] |
There was a problem hiding this comment.
Great if this fixes the hangs we were seeing on Mac.
|
Related to #34880 |
I think I'm gonna frame that for you 😄 |
| /// </summary> | ||
| internal static class NamedPipeUtil | ||
| { | ||
| // Size of the buffers to use: 64K |
There was a problem hiding this comment.
The Pipe buffers, of course 😉
| // can be quite long, leaving very little room for the actual pipe name. Fortunately, | ||
| // '/tmp' is mandated by POSIX to always be a valid temp directory, so we can use that | ||
| // instead. | ||
| return Path.Combine("/tmp", pipeName); |
There was a problem hiding this comment.
Didn't @kg mention that some mono scenarios didn't have a /tmp directory?
There was a problem hiding this comment.
I'm not aware of these situations, but someone else brought it up on one of my PRs
There was a problem hiding this comment.
Well, there are technically two things to separate here: whether /tmp exists, and whether it is the system temporary directory.
POSIX requires /tmp exist, so I think almost all reasonable platforms should have it.
However, no system is required to choose /tmp as their system temporary directory. The most prominent example here is macOS, where they have a randomly generated one.
In this case I'm assuming that even if /tmp isn't the preferred system temporary directory, it's a valid directory. I think this is a reasonable assumption and probably shouldn't have severe consequences otherwise. If not for the UDS path length limit I would just use the standard system temporary directory.
PublishReadyToRun=true has no effect until we upgrade to netcoreapp3.0 or above. Unity no longer supports OSX El Capitan, so net46 support is no longer needed. The pipe-too-long issue was fixed in dotnet#34607 The src/Compilers/Shared/RuntimeHostInfo.cs change is equivalent to a PR that has already been merged into Roslyn, and should go away once we update to something including that.
The bulk of this change is in the second commit. The first commit is just changing some of the compiler server test helpers to propagate exceptions instead of crashing the process so it's easier to diagnose CI test failures.
The default behavior for the NamedPipeServerStream API is to take a pipe
name and then construct a named pipe in the background using that name.
In Windows this involves creating a file in a special namespace in the
file system. On Unix, named pipes are implemented using Unix Domain
Sockets, which are actual files, and the CoreFX behavior is to create
them in the temporary directory. Unfortunately, Unix Domain Sockets also
often have a max path length limitation and the temporary directory
could be arbitrarily long, meaning that any attempt to create a named
pipe may fail on Unix.
To remedy this, CoreFX introduced an API which allows you to pass a full
path instead of just a pipe name. If a fully-qualifed path is passed,
the new behavior is used. We can use this functionality to improve
reliability of pipe name creation by using the "/tmp" directory on Unix,
which by the POSIX specification is always required to be a valid
temporary directory, and by using a fixed-length pipe name that is lower
than any known Unix Domain Socket path length restriction.
This should fix the issue for good.
Fixes #24137