Merge pull request #38688 from jaredpar/fix-sync

Additional logging in the compiler server

src/Compilers/Shared/BuildServerConnection.cs

@@ -125,79 +125,109 @@ internal sealed class BuildServerConnection
                 return new IncorrectHashBuildResponse();
             }
 
-            var clientDir = buildPaths.ClientDirectory;
-            var timeoutNewProcess = timeoutOverride ?? TimeOutMsNewProcess;
-            var timeoutExistingProcess = timeoutOverride ?? TimeOutMsExistingProcess;
-            Task<NamedPipeClientStream> pipeTask = null;
-            IServerMutex clientMutex = null;
-            try
+            var pipeTask = tryConnectToServer(pipeName, buildPaths, timeoutOverride, createServerFunc, cancellationToken);
+            if (pipeTask is null)
             {
-                var holdsMutex = false;
-                try
+                return new RejectedBuildResponse();
+            }
+            else
+            {
+                var pipe = await pipeTask.ConfigureAwait(false);
+                if (pipe is null)
                 {
-                    var clientMutexName = GetClientMutexName(pipeName);
-                    clientMutex = OpenOrCreateMutex(clientMutexName, out holdsMutex);
+                    return new RejectedBuildResponse();
                 }
-                catch
+                else
                 {
-                    // The Mutex constructor can throw in certain cases. One specific example is docker containers
-                    // where the /tmp directory is restricted. In those cases there is no reliable way to execute
-                    // the server and we need to fall back to the command line.
-                    //
-                    // Example: https://github.com/dotnet/roslyn/issues/24124
-                    return new RejectedBuildResponse();
+                    var request = BuildRequest.Create(language,
+                                                      buildPaths.WorkingDirectory,
+                                                      buildPaths.TempDirectory,
+                                                      BuildProtocolConstants.GetCommitHash(),
+                                                      arguments,
+                                                      keepAlive,
+                                                      libEnvVariable);
+
+                    return await TryCompile(pipe, request, cancellationToken).ConfigureAwait(false);
                 }
+            }
 
-                if (!holdsMutex)
+            // This code uses a Mutex.WaitOne / ReleaseMutex pairing. Both of these calls must occur on the same thread 
+            // or an exception will be thrown. This code lives in a separate non-async function to help ensure this 
+            // invariant doesn't get invalidated in the future by an `await` being inserted. 
+            static Task<NamedPipeClientStream> tryConnectToServer(
+                string pipeName,
+                BuildPathsAlt buildPaths,
+                int? timeoutOverride,
+                CreateServerFunc createServerFunc,
+                CancellationToken cancellationToken)
+            {
+                var originalThreadId = Thread.CurrentThread.ManagedThreadId;
+                var clientDir = buildPaths.ClientDirectory;
+                var timeoutNewProcess = timeoutOverride ?? TimeOutMsNewProcess;
+                var timeoutExistingProcess = timeoutOverride ?? TimeOutMsExistingProcess;
+                Task<NamedPipeClientStream> pipeTask = null;
+                IServerMutex clientMutex = null;
+                try
                 {
+                    var holdsMutex = false;
                     try
                     {
-                        holdsMutex = clientMutex.TryLock(timeoutNewProcess);
+                        var clientMutexName = GetClientMutexName(pipeName);
+                        clientMutex = OpenOrCreateMutex(clientMutexName, out holdsMutex);
+                    }
+                    catch
+                    {
+                        // The Mutex constructor can throw in certain cases. One specific example is docker containers
+                        // where the /tmp directory is restricted. In those cases there is no reliable way to execute
+                        // the server and we need to fall back to the command line.
+                        //
+                        // Example: https://github.com/dotnet/roslyn/issues/24124
+                        return null;
+                    }
 
-                        if (!holdsMutex)
+                    if (!holdsMutex)
+                    {
+                        try
                         {
-                            return new RejectedBuildResponse();
+                            holdsMutex = clientMutex.TryLock(timeoutNewProcess);
+
+                            if (!holdsMutex)
+                            {
+                                return null;
+                            }
+                        }
+                        catch (AbandonedMutexException)
+                        {
+                            holdsMutex = true;
                         }
                     }
-                    catch (AbandonedMutexException)
+
+                    // Check for an already running server
+                    var serverMutexName = GetServerMutexName(pipeName);
+                    bool wasServerRunning = WasServerMutexOpen(serverMutexName);
+                    var timeout = wasServerRunning ? timeoutExistingProcess : timeoutNewProcess;
+
+                    if (wasServerRunning || createServerFunc(clientDir, pipeName))
                     {
-                        holdsMutex = true;
+                        pipeTask = TryConnectToServerAsync(pipeName, timeout, cancellationToken);
                     }
-                }
-
-                // Check for an already running server
-                var serverMutexName = GetServerMutexName(pipeName);
-                bool wasServerRunning = WasServerMutexOpen(serverMutexName);
-                var timeout = wasServerRunning ? timeoutExistingProcess : timeoutNewProcess;
 
-                if (wasServerRunning || createServerFunc(clientDir, pipeName))
-                {
-                    pipeTask = TryConnectToServerAsync(pipeName, timeout, cancellationToken);
+                    return pipeTask;
                 }
-            }
-            finally
-            {
-                clientMutex?.Dispose();
-            }
-
-            if (pipeTask != null)
-            {
-                var pipe = await pipeTask.ConfigureAwait(false);
-                if (pipe != null)
+                finally
                 {
-                    var request = BuildRequest.Create(language,
-                                                      buildPaths.WorkingDirectory,
-                                                      buildPaths.TempDirectory,
-                                                      BuildProtocolConstants.GetCommitHash(),
-                                                      arguments,
-                                                      keepAlive,
-                                                      libEnvVariable);
-
-                    return await TryCompile(pipe, request, cancellationToken).ConfigureAwait(false);
+                    try
+                    {
+                        clientMutex?.Dispose();
+                    }
+                    catch (ApplicationException e)
+                    {
+                        var releaseThreadId = Thread.CurrentThread.ManagedThreadId;
+                        var message = $"ReleaseMutex failed. WaitOne Id: {originalThreadId} Release Id: {releaseThreadId}";
+                        throw new Exception(message, e);
+                    }
                 }
             }
-
-            return new RejectedBuildResponse();
         }
 
         /// <summary>
@@ -601,7 +631,6 @@ public static string GetTempPath(string workingDir)
     internal interface IServerMutex : IDisposable
     {
         bool TryLock(int timeoutMs);
-        void Unlock();
         bool IsDisposed { get; }
     }
 
@@ -732,16 +761,6 @@ public bool TryLock(int timeoutMs)
             return IsLocked = Mutex.WaitOne(timeoutMs);
         }
 
-        public void Unlock()
-        {
-            if (IsDisposed)
-                throw new ObjectDisposedException("Mutex");
-            if (!IsLocked)
-                throw new InvalidOperationException("Lock not held");
-            Mutex.ReleaseMutex();
-            IsLocked = false;
-        }
-
         public void Dispose()
         {
             if (IsDisposed)
@@ -756,6 +775,7 @@ public void Dispose()
             finally
             {
                 Mutex.Dispose();
+                IsLocked = false;
             }
         }
     }
@@ -792,13 +812,6 @@ public bool TryLock(int timeoutMs)
             return HeldMutex.TryLock(timeoutMs);
         }
 
-        public void Unlock()
-        {
-            if (IsDisposed)
-                throw new ObjectDisposedException("Mutex");
-            HeldMutex.Unlock();
-        }
-
         public void Dispose()
         {
             if (IsDisposed)