From 210c17ea60f525837a7525df73e7332598ad4089 Mon Sep 17 00:00:00 2001 Patch-Source: https://github.com/dotnet/roslyn/pull/57003 From: Antoine Martin Date: Sat, 27 Aug 2022 21:26:01 -0800 Subject: [PATCH 1/1] mono-named-mutex --- .../InternalUtilities/PlatformInformation.cs | 19 ++ .../VBCSCompilerTests/BuildClientTests.cs | 2 +- .../CompilerServerApiTest.cs | 4 +- .../VBCSCompilerTests/CompilerServerTests.cs | 4 +- .../VBCSCompilerServerTests.cs | 7 +- src/Compilers/Shared/BuildServerConnection.cs | 251 +++++++++++------- 6 files changed, 182 insertions(+), 105 deletions(-) diff --git a/src/roslyn/src/Compilers/Core/Portable/InternalUtilities/PlatformInformation.cs b/src/roslyn/src/Compilers/Core/Portable/InternalUtilities/PlatformInformation.cs index 033e66cd2f2..d4fa56413fb 100644 --- a/src/roslyn/src/Compilers/Core/Portable/InternalUtilities/PlatformInformation.cs +++ b/src/roslyn/src/Compilers/Core/Portable/InternalUtilities/PlatformInformation.cs @@ -31,5 +31,24 @@ public static bool IsRunningOnMono } } } + /// + /// Are we running on .NET 5 or later using the Mono runtime? + /// Will also return true when running on Mono itself; if necessary + /// we can use IsRunningOnMono to distinguish. + /// + public static bool IsUsingMonoRuntime + { + get + { + try + { + return !(Type.GetType("Mono.RuntimeStructs", throwOnError: false) is null); + } + catch + { + return false; + } + } + } } } diff --git a/src/roslyn/src/Compilers/Server/VBCSCompilerTests/BuildClientTests.cs b/src/roslyn/src/Compilers/Server/VBCSCompilerTests/BuildClientTests.cs index 669d1bfb676..7f1d0468823 100644 --- a/src/roslyn/src/Compilers/Server/VBCSCompilerTests/BuildClientTests.cs +++ b/src/roslyn/src/Compilers/Server/VBCSCompilerTests/BuildClientTests.cs @@ -79,7 +79,7 @@ public void ConnectToServerFails() // to connect. When it fails it should fall back to in-proc // compilation. bool holdsMutex; - using (var serverMutex = new Mutex(initiallyOwned: true, + using (var serverMutex = BuildServerConnection.OpenOrCreateMutex( name: BuildServerConnection.GetServerMutexName(_pipeName), createdNew: out holdsMutex)) { diff --git a/src/roslyn/src/Compilers/Server/VBCSCompilerTests/CompilerServerApiTest.cs b/src/roslyn/src/Compilers/Server/VBCSCompilerTests/CompilerServerApiTest.cs index 0dbd1b2e143..c00b72e8434 100644 --- a/src/roslyn/src/Compilers/Server/VBCSCompilerTests/CompilerServerApiTest.cs +++ b/src/roslyn/src/Compilers/Server/VBCSCompilerTests/CompilerServerApiTest.cs @@ -103,7 +103,7 @@ public void MutexStopsServerStarting() var mutexName = BuildServerConnection.GetServerMutexName(pipeName); bool holdsMutex; - using (var mutex = new Mutex(initiallyOwned: true, + using (var mutex = BuildServerConnection.OpenOrCreateMutex( name: mutexName, createdNew: out holdsMutex)) { @@ -119,7 +119,7 @@ public void MutexStopsServerStarting() } finally { - mutex.ReleaseMutex(); + mutex.Dispose(); } } } diff --git a/src/roslyn/src/Compilers/Server/VBCSCompilerTests/CompilerServerTests.cs b/src/roslyn/src/Compilers/Server/VBCSCompilerTests/CompilerServerTests.cs index 3b6f4f291ff..e97a6bf923f 100644 --- a/src/roslyn/src/Compilers/Server/VBCSCompilerTests/CompilerServerTests.cs +++ b/src/roslyn/src/Compilers/Server/VBCSCompilerTests/CompilerServerTests.cs @@ -304,7 +304,7 @@ public async Task ServerFailsWithLongTempPathUnix() var newTempDir = _tempDirectory.CreateDirectory(new string('a', 100 - _tempDirectory.Path.Length)); await ApplyEnvironmentVariables( new[] { new KeyValuePair("TMPDIR", newTempDir.Path) }, - async () => + async () => await Task.Run(async () => { using var serverData = await ServerUtil.CreateServer(_logger); var result = RunCommandLineCompiler( @@ -317,7 +317,7 @@ public async Task ServerFailsWithLongTempPathUnix() var listener = await serverData.Complete(); Assert.Equal(CompletionData.RequestCompleted, listener.CompletionDataList.Single()); - }); + })); } [Fact] diff --git a/src/roslyn/src/Compilers/Server/VBCSCompilerTests/VBCSCompilerServerTests.cs b/src/roslyn/src/Compilers/Server/VBCSCompilerTests/VBCSCompilerServerTests.cs index d5f493fed8a..73941972e48 100644 --- a/src/roslyn/src/Compilers/Server/VBCSCompilerTests/VBCSCompilerServerTests.cs +++ b/src/roslyn/src/Compilers/Server/VBCSCompilerTests/VBCSCompilerServerTests.cs @@ -101,7 +101,7 @@ public async Task NoServerConnection() var thread = new Thread(() => { - using (var mutex = new Mutex(initiallyOwned: true, name: mutexName, createdNew: out created)) + using (var mutex = BuildServerConnection.OpenOrCreateMutex(name: mutexName, createdNew: out created)) using (var stream = NamedPipeUtil.CreateServer(pipeName)) { readyMre.Set(); @@ -112,7 +112,7 @@ public async Task NoServerConnection() stream.Close(); doneMre.WaitOne(); - mutex.ReleaseMutex(); + mutex.Dispose(); } }); @@ -153,7 +153,7 @@ public async Task ServerShutdownsDuringProcessing() { using (var stream = NamedPipeUtil.CreateServer(pipeName)) { - var mutex = new Mutex(initiallyOwned: true, name: mutexName, createdNew: out created); + var mutex = BuildServerConnection.OpenOrCreateMutex(name: mutexName, createdNew: out created); readyMre.Set(); stream.WaitForConnection(); @@ -161,7 +161,6 @@ public async Task ServerShutdownsDuringProcessing() // Client is waiting for a response. Close the mutex now. Then close the connection // so the client gets an error. - mutex.ReleaseMutex(); mutex.Dispose(); stream.Close(); diff --git a/src/roslyn/src/Compilers/Shared/BuildServerConnection.cs b/src/roslyn/src/Compilers/Shared/BuildServerConnection.cs index f67c2d83957..1fe609061ee 100644 --- a/src/roslyn/src/Compilers/Shared/BuildServerConnection.cs +++ b/src/roslyn/src/Compilers/Shared/BuildServerConnection.cs @@ -543,19 +543,10 @@ internal static bool WasServerMutexOpen(string mutexName) { try { - if (PlatformInformation.IsRunningOnMono) + if (PlatformInformation.IsUsingMonoRuntime) { - IServerMutex? mutex = null; - bool createdNew = false; - try - { - mutex = new ServerFileMutexPair(mutexName, false, out createdNew); - return !createdNew; - } - finally - { - mutex?.Dispose(); - } + using var mutex = new ServerFileMutex(mutexName); + return !mutex.CouldLock(); } else { @@ -572,9 +563,11 @@ internal static bool WasServerMutexOpen(string mutexName) internal static IServerMutex OpenOrCreateMutex(string name, out bool createdNew) { - if (PlatformInformation.IsRunningOnMono) + if (PlatformInformation.IsUsingMonoRuntime) { - return new ServerFileMutexPair(name, initiallyOwned: true, out createdNew); + var mutex = new ServerFileMutex(name); + createdNew = mutex.TryLock(0); + return mutex; } else { @@ -648,19 +641,22 @@ internal interface IServerMutex : IDisposable } /// - /// An interprocess mutex abstraction based on OS advisory locking (FileStream.Lock/Unlock). + /// An interprocess mutex abstraction based on file sharing permission (FileShare.None). /// If multiple processes running as the same user create FileMutex instances with the same name, /// those instances will all point to the same file somewhere in a selected temporary directory. - /// The TryLock method can be used to attempt to acquire the mutex, with Unlock or Dispose used to release. + /// The TryLock method can be used to attempt to acquire the mutex, with Dispose used to release. + /// The CouldLock method can be used to check whether an attempt to acquire the mutex would have + /// succeeded at the current time, without actually acquiring it. /// Unlike Win32 named mutexes, there is no mechanism for detecting an abandoned mutex. The file /// will simply revert to being unlocked but remain where it is. /// - internal sealed class FileMutex : IDisposable + internal sealed class ServerFileMutex : IServerMutex { - public readonly FileStream Stream; + public FileStream? Stream; public readonly string FilePath; + public readonly string GuardPath; - public bool IsLocked { get; private set; } + public bool IsDisposed { get; private set; } internal static string GetMutexDirectory() { @@ -670,61 +666,176 @@ internal static string GetMutexDirectory() return result; } - public FileMutex(string name) + public ServerFileMutex(string name) { - FilePath = Path.Combine(GetMutexDirectory(), name); - Stream = new FileStream(FilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None); + var mutexDirectory = GetMutexDirectory(); + FilePath = Path.Combine(mutexDirectory, name); + GuardPath = Path.Combine(mutexDirectory, ".guard"); } - public bool TryLock(int timeoutMs) + /// + /// Acquire the guard by opening the guard file with FileShare.None. The guard must only ever + /// be held for very brief amounts of time, so we can simply spin until it is acquired. The + /// guard must be released by disposing the FileStream returned from this routine. Note the + /// guard file is never deleted; this is a leak, but only of a single file. + /// + internal FileStream LockGuard() { - if (IsLocked) - throw new InvalidOperationException("Lock already held"); - - var sw = Stopwatch.StartNew(); - do + // We should be able to acquire the guard quickly. Limit the number of retries anyway + // by some arbitrary bound to avoid getting hung up in a possibly infinite loop. + for (var i = 0; i < 100; i++) { try { - Stream.Lock(0, 0); - IsLocked = true; - return true; + return new FileStream(GuardPath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None); } catch (IOException) { - // Lock currently held by someone else. + // Guard currently held by someone else. // We want to sleep for a short period of time to ensure that other processes // have an opportunity to finish their work and relinquish the lock. // Spinning here (via Yield) would work but risks creating a priority // inversion if the lock is held by a lower-priority process. Thread.Sleep(1); } + } + // Handle unexpected failure to acquire guard as error. + throw new InvalidOperationException("Unable to acquire guard"); + } + + /// + /// Attempt to acquire the lock by opening the lock file with FileShare.None. Sets "Stream" + /// and returns true if successful, returns false if the lock is already held by another + /// thread or process. Guard must be held when calling this routine. + /// + internal bool TryLockFile() + { + Debug.Assert(Stream is null); + FileStream? stream = null; + try + { + stream = new FileStream(FilePath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None); + // On some targets, the file locking used to implement FileShare.None may not be + // atomic with opening/creating the file. This creates a race window when another + // thread holds the lock and is just about to unlock: we may be able to open the + // file here, then the other thread unlocks and deletes the file, and then we + // acquire the lock on our file handle - but the actual file is already deleted. + // To close this race, we verify that the file does in fact still exist now that + // we have successfull acquired the locked FileStream. (Note that this check is + // safe because we cannot race with an other attempt to create the file since we + // hold the guard, and after the FileStream constructor returned we can no race + // with file deletion because we hold the lock.) + if (!File.Exists(FilePath)) + { + // To simplify the logic, we treat this case as "unable to acquire the lock" + // because it we caught another process while it owned the lock and was just + // giving it up. If the caller retries, we'll likely acquire the lock then. + stream.Dispose(); + return false; + } + } + catch (Exception) + { + stream?.Dispose(); + return false; + } + Stream = stream; + return true; + } + + /// + /// Release the lock by deleting the lock file and disposing "Stream". + /// + internal void UnlockFile() + { + Debug.Assert(Stream is not null); + try + { + // Delete the lock file while the stream is not yet disposed + // and we therefore still hold the FileShare.None exclusion. + // There may still be a race with another thread attempting a + // TryLockFile in parallel, but that is safely handled there. + File.Delete(FilePath); + } + finally + { + Stream.Dispose(); + Stream = null; + } + } + + public bool TryLock(int timeoutMs) + { + if (IsDisposed) + throw new ObjectDisposedException("Mutex"); + if (Stream is not null) + throw new InvalidOperationException("Lock already held"); + + var sw = Stopwatch.StartNew(); + do + { + try + { + // Attempt to acquire lock while holding guard. + using var guard = LockGuard(); + if (TryLockFile()) + return true; + } catch (Exception) { - // Something else went wrong. return false; } + + // See comment in LockGuard. + Thread.Sleep(1); } while (sw.ElapsedMilliseconds < timeoutMs); return false; } - public void Unlock() + public bool CouldLock() { - if (!IsLocked) - return; - Stream.Unlock(0, 0); - IsLocked = false; + if (IsDisposed) + return false; + if (Stream is not null) + return false; + + try + { + // Attempt to acquire lock while holding guard, and if successful + // immediately unlock again while still holding guard. This ensures + // no other thread will spuriously observe the lock as held due to + // the lock attempt here. + using var guard = LockGuard(); + if (TryLockFile()) + { + UnlockFile(); + return true; + } + } + catch (Exception) + { + return false; + } + + return false; } public void Dispose() { - var wasLocked = IsLocked; - if (wasLocked) - Unlock(); - Stream.Dispose(); - // We do not delete the lock file here because there is no reliable way to perform a - // 'delete if no one has the file open' operation atomically on *nix. This is a leak. + if (IsDisposed) + return; + IsDisposed = true; + if (Stream is not null) + { + try + { + UnlockFile(); + } + catch (Exception) + { + } + } } } @@ -792,56 +903,4 @@ public void Dispose() } } } - - /// - /// Approximates a named mutex with 'locked', 'unlocked' and 'abandoned' states. - /// There is no reliable way to detect whether a mutex has been abandoned on some target platforms, - /// so we use the AliveMutex to manually track whether the creator of a mutex is still running, - /// while the HeldMutex represents the actual lock state of the mutex. - /// - internal sealed class ServerFileMutexPair : IServerMutex - { - public readonly FileMutex AliveMutex; - public readonly FileMutex HeldMutex; - - public bool IsDisposed { get; private set; } - - public ServerFileMutexPair(string mutexName, bool initiallyOwned, out bool createdNew) - { - AliveMutex = new FileMutex(mutexName + "-alive"); - HeldMutex = new FileMutex(mutexName + "-held"); - createdNew = AliveMutex.TryLock(0); - if (initiallyOwned && createdNew) - { - if (!TryLock(0)) - throw new Exception("Failed to lock mutex after creating it"); - } - } - - public bool TryLock(int timeoutMs) - { - if (IsDisposed) - throw new ObjectDisposedException("Mutex"); - return HeldMutex.TryLock(timeoutMs); - } - - public void Dispose() - { - if (IsDisposed) - return; - IsDisposed = true; - - try - { - HeldMutex.Unlock(); - AliveMutex.Unlock(); - } - finally - { - AliveMutex.Dispose(); - HeldMutex.Dispose(); - } - } - } - } -- 2.36.2