PR feedback

src/Compilers/Core/MSBuildTask/ValidateBootstrap.cs

@@ -72,6 +72,16 @@ public override bool Execute()
                 }
             }
 
+            // This represents the maximum number of failed build attempts on the server before we will declare
+            // that the overall build itself failed. 
+            //
+            // The goal is to keep this at zero. The errors here are a mix of repository construction errors (having
+            // incompatible NuGet analyzers) and product errors (having flaky behavior in the server). Any time this
+            // number goes above zero it means we are dropping connections during developer inner loop builds and 
+            // hence measurably slowing down our productivity.
+            //
+            // When we find issues in the server or our infra we can temporarily raise this number while it is
+            // being worked out but should file a bug to track getting this to zero.
             const int maxRejectCount = 0;
             var rejectCount = 0;
             foreach (var tuple in s_failedQueue.ToList())

src/Compilers/Server/VBCSCompiler/ClientConnectionHandler.cs

@@ -103,7 +103,7 @@ async Task<CompletionData> ProcessCore()
             }
         }
 
-        private async Task<CompletionData> WriteBuildResponseAsync(IClientConnection clientConnection, BuildResponse response, CompletionData completionData, CancellationToken cancellationToken)
+        private static async Task<CompletionData> WriteBuildResponseAsync(IClientConnection clientConnection, BuildResponse response, CompletionData completionData, CancellationToken cancellationToken)
         {
             var message = response switch
             {

src/Compilers/Server/VBCSCompiler/IClientConnection.cs

@@ -41,6 +41,10 @@ internal interface IClientConnection : IDisposable
 
     internal interface IClientConnectionHost
     {
+        /// <summary>
+        /// True when the host is listening for new connections (after <see cref="BeginListening"/> is
+        /// called but before <see cref="EndListening"/> is called).
+        /// </summary>
         bool IsListening { get; }
 
         /// <summary>

src/Compilers/Server/VBCSCompiler/NamedPipeClientConnectionHost.cs

@@ -2,21 +2,17 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
-using Roslyn.Utilities;
 using System;
+using System.Collections.Generic;
+using System.Collections.Immutable;
 using System.Diagnostics;
-using System.IO;
 using System.IO.Pipes;
-using System.Runtime.CompilerServices;
-using System.Security.Principal;
+using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.CommandLine;
-using System.Security.AccessControl;
 using Microsoft.CodeAnalysis.Diagnostics;
-using System.Collections.Immutable;
-using System.Linq;
-using System.Collections.Generic;
+using Roslyn.Utilities;
 
 #nullable enable
 
@@ -38,7 +34,7 @@ internal ListenResult(NamedPipeClientConnection? connection = null, Exception? e
         }
 
         private CancellationTokenSource? _cancellationTokenSource;
-        private ImmutableArray<Task> _listenTasks;
+        private Task[]? _listenTasks;
         private AsyncQueue<ListenResult>? _queue;
 
         public string PipeName { get; }
@@ -57,7 +53,7 @@ public void BeginListening()
             }
 
             Debug.Assert(_cancellationTokenSource is null);
-            Debug.Assert(_listenTasks.IsDefault);
+            Debug.Assert(_listenTasks is null);
             Debug.Assert(_queue is null);
 
             IsListening = true;
@@ -72,14 +68,13 @@ public void BeginListening()
             // Should you ever want to change this number in the future make sure to test the new values on sufficiently
             // large builds such as dotnet/roslyn or dotnet/runtime
             var listenCount = Math.Min(4, Environment.ProcessorCount);
-            var listenTasks = new List<Task>(capacity: listenCount);
+            _listenTasks = new Task[listenCount];
             int clientLoggingIdentifier = 0;
             for (int i = 0; i < listenCount; i++)
             {
                 var task = Task.Run(() => ListenCoreAsync(PipeName, _queue, GetNextClientLoggingIdentifier, _cancellationTokenSource.Token));
-                listenTasks.Add(task);
+                _listenTasks[i] = task;
             }
-            _listenTasks = ImmutableArray.CreateRange(listenTasks);
 
             string GetNextClientLoggingIdentifier()
             {
@@ -97,16 +92,16 @@ public void EndListening()
 
             Debug.Assert(_cancellationTokenSource is object);
             Debug.Assert(_queue is object);
-            Debug.Assert(!_listenTasks.IsDefault);
+            Debug.Assert(_listenTasks is object);
 
             _cancellationTokenSource.Cancel();
             try
             {
-                Task.WaitAll(_listenTasks.ToArray());
+                Task.WaitAll(_listenTasks);
             }
             catch (Exception ex)
             {
-                CompilerServerLogger.LogException(ex, "Listen tasks threw exception during EndListen");
+                CompilerServerLogger.LogException(ex, $"Listen tasks threw exception during nameof(EndListening");
             }
 
             _queue.Complete();
@@ -122,7 +117,7 @@ public void EndListening()
             _queue = null;
             _cancellationTokenSource.Dispose();
             _cancellationTokenSource = null;
-            _listenTasks = default;
+            _listenTasks = null;
             IsListening = false;
         }
 

src/Compilers/Server/VBCSCompiler/ServerDispatcher.cs

@@ -168,7 +168,7 @@ private void CheckCompletedTasks(CancellationToken cancellationToken)
 
             if (_gcTask?.IsCompleted == true)
             {
-                RunGargbageCollection();
+                RunGC();
             }
 
             HandleCompletedConnections();
@@ -221,7 +221,7 @@ private void ChangeToShuttingDown(string reason)
             _gcTask = null;
         }
 
-        private void RunGargbageCollection()
+        private void RunGC()
         {
             _gcTask = null;
             for (int i = 0; i < 10; i++)
@@ -235,7 +235,7 @@ private void RunGargbageCollection()
 
         private void MaybeCreateListenTask()
         {
-            if (_listenTask is null && _clientConnectionHost.IsListening)
+            if (_listenTask is null)
             {
                 _listenTask = _clientConnectionHost.GetNextClientConnectionAsync();
             }

src/Compilers/Server/VBCSCompilerTests/BuildClientTests.cs

@@ -139,11 +139,11 @@ public async Task ConnectToPipe()
 
                 var oneSec = TimeSpan.FromSeconds(1);
 
-                Assert.False(await TryConnectToNamedPipe((int)oneSec.TotalMilliseconds, cancellationToken: default));
+                Assert.False(await tryConnectToNamedPipe((int)oneSec.TotalMilliseconds, cancellationToken: default));
 
                 // Try again with infinite timeout and cancel
                 var cts = new CancellationTokenSource();
-                var connection = TryConnectToNamedPipe(Timeout.Infinite, cts.Token);
+                var connection = tryConnectToNamedPipe(Timeout.Infinite, cts.Token);
                 Assert.False(connection.IsCompleted);
                 cts.Cancel();
                 await Assert.ThrowsAnyAsync<OperationCanceledException>(
@@ -151,9 +151,9 @@ public async Task ConnectToPipe()
 
                 // Create server and try again
                 Assert.True(TryCreateServer(pipeName));
-                Assert.True(await TryConnectToNamedPipe(Timeout.Infinite, cancellationToken: default));
+                Assert.True(await tryConnectToNamedPipe(Timeout.Infinite, cancellationToken: default));
 
-                async Task<bool> TryConnectToNamedPipe(int timeoutMs, CancellationToken cancellationToken)
+                async Task<bool> tryConnectToNamedPipe(int timeoutMs, CancellationToken cancellationToken)
                 {
                     using var pipeStream = await BuildServerConnection.TryConnectToServerAsync(pipeName, timeoutMs, cancellationToken);
                     return pipeStream != null;

src/Compilers/Server/VBCSCompilerTests/BuildServerControllerTests.cs

@@ -13,7 +13,7 @@ public sealed class BuildServerControllerTests : IDisposable
 
         public void Dispose()
         {
-            HackUtil.DisposeAll();
+            NamedPipeTestUtil.DisposeAll();
 
         }
 

src/Compilers/Server/VBCSCompilerTests/NamedPipeClientConnectionHostTests.cs

@@ -15,7 +15,7 @@ namespace Microsoft.CodeAnalysis.CompilerServer.UnitTests
 {
     public class NamedPipeClientConnectionHostTests : IDisposable
     {
-        private NamedPipeClientConnectionHost _host;
+        private readonly NamedPipeClientConnectionHost _host;
 
         public NamedPipeClientConnectionHostTests()
         {
@@ -29,7 +29,7 @@ public void Dispose()
                 _host.EndListening();
             }
 
-            HackUtil.AssertPipeFullyClosed(_host.PipeName);
+            Assert.True(NamedPipeTestUtil.IsPipeFullyClosed(_host.PipeName));
         }
 
         private Task<NamedPipeClientStream> ConnectAsync(CancellationToken cancellationToken = default) => BuildServerConnection.TryConnectToServerAsync(

src/Compilers/Server/VBCSCompilerTests/HackUtil.cs → src/Compilers/Server/VBCSCompilerTests/NamedPipeTestUtil.cs

-// Licensed to the .NET Foundation under one or more agreements.
+// Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
@@ -10,7 +10,6 @@
 using System.Reflection;
 using System.Net.Sockets;
 using Roslyn.Test.Utilities;
-using Xunit;
 
 #nullable enable
 
@@ -24,7 +23,7 @@ namespace Microsoft.CodeAnalysis.CompilerServer.UnitTests
     ///
     /// Do NOT check this into production, it's a unit test utility only
     /// </summary>
-    internal static class HackUtil
+    internal static class NamedPipeTestUtil
     {
         private static IDictionary GetSharedServersDictionary()
         {
@@ -74,11 +73,7 @@ private static Socket GetSocket(object sharedServer)
             }
         }
 
-        internal static void AssertPipeFullyClosed(string pipeName)
-        {
-            var socket = GetSocketForPipeName(pipeName);
-            Assert.Null(socket);
-        }
+        internal static bool IsPipeFullyClosed(string pipeName) => GetSocketForPipeName(pipeName) is null;
 
         internal static void DisposeAll()
         {

src/Compilers/Server/VBCSCompilerTests/ServerUtil.cs

@@ -86,7 +86,7 @@ public void Dispose()
             }
 
             ServerTask.Wait();
-            HackUtil.AssertPipeFullyClosed(PipeName);
+            Assert.True(NamedPipeTestUtil.IsPipeFullyClosed(PipeName));
         }
     }
 

src/Compilers/Server/VBCSCompilerTests/VBCSCompilerServerTests.cs

@@ -334,7 +334,7 @@ public async Task CompilationExceptionShouldShutdown()
             {
                 var hitCompilation = false;
                 var compilerServerHost = new TestableCompilerServerHost(delegate
-                { 
+                {
                     hitCompilation = true;
                     throw new Exception("");
                 });
@@ -354,7 +354,7 @@ public async Task CompilationExceptionShouldShutdown()
             public async Task AnalyzerInconsistencyShouldShutdown()
             {
                 var compilerServerHost = new TestableCompilerServerHost(delegate
-                { 
+                {
                     return new AnalyzerInconsistencyBuildResponse();
                 });