From f4598558da32ddc0587c8566dd69dc85a5661109 Mon Sep 17 00:00:00 2001 From: Jared Parsons Date: Fri, 25 Dec 2015 17:00:47 -0800 Subject: [PATCH] Test shutdown in middle of compilation Added a test to verify the shutdown operation is passive. If any existing compilations are in progress they will be allowed to complete after the shutdown is received. This required a bit of code refactoring. It needed to be easier to intercept compilation requests and block on them until a given set of events had completed. --- .../CommandLine/TouchedFileLoggingTests.cs | 2 +- .../ServerShared/CSharpCompilerServer.cs | 8 +-- .../ServerShared/CompilerRequestHandler.cs | 40 ++++++++++++- .../Server/ServerShared/Connection.cs | 48 ++++------------ .../ServerShared/ICompilerServerHost.cs | 5 +- .../ServerShared/VisualBasicCompilerServer.cs | 8 +-- .../VBCSCompiler/DesktopCompilerServerHost.cs | 4 +- .../CompilerServerApiTest.cs | 57 +++++++++++++++++++ .../VBCSCompilerTests/CompilerServerTests.cs | 20 ------- .../Server/VBCSCompilerTests/ServerUtil.cs | 21 +++++++ .../TestableCompilerServerHost.cs | 23 ++++++++ .../VBCSCompilerTests.csproj | 1 + .../CommandLine/TouchedFileLoggingTests.vb | 2 +- 13 files changed, 164 insertions(+), 75 deletions(-) create mode 100644 src/Compilers/Server/VBCSCompilerTests/TestableCompilerServerHost.cs diff --git a/src/Compilers/CSharp/Test/CommandLine/TouchedFileLoggingTests.cs b/src/Compilers/CSharp/Test/CommandLine/TouchedFileLoggingTests.cs index 533ee1eb0af..ae700c0cf84 100644 --- a/src/Compilers/CSharp/Test/CommandLine/TouchedFileLoggingTests.cs +++ b/src/Compilers/CSharp/Test/CommandLine/TouchedFileLoggingTests.cs @@ -215,7 +215,7 @@ public void TrivialMetadataCaching() filelist.Add(source1); var outWriter = new StringWriter(); var cmd = new CSharpCompilerServer( - new DesktopCompilerServerHost(), + DesktopCompilerServerHost.SharedAssemblyReferenceProvider, new[] { "/nologo", "/touchedfiles:" + touchedBase, source1 }, null, _baseDirectory, diff --git a/src/Compilers/Server/ServerShared/CSharpCompilerServer.cs b/src/Compilers/Server/ServerShared/CSharpCompilerServer.cs index 828068dbbf9..77d37074c74 100644 --- a/src/Compilers/Server/ServerShared/CSharpCompilerServer.cs +++ b/src/Compilers/Server/ServerShared/CSharpCompilerServer.cs @@ -13,17 +13,17 @@ namespace Microsoft.CodeAnalysis.CompilerServer { internal sealed class CSharpCompilerServer : CSharpCompiler { - private readonly ICompilerServerHost _compilerServerHost; + private readonly Func _metadataProvider; - internal CSharpCompilerServer(ICompilerServerHost compilerServerHost, string[] args, string clientDirectory, string baseDirectory, string sdkDirectory, string libDirectory, IAnalyzerAssemblyLoader analyzerLoader) + internal CSharpCompilerServer(Func metadataProvider, string[] args, string clientDirectory, string baseDirectory, string sdkDirectory, string libDirectory, IAnalyzerAssemblyLoader analyzerLoader) : base(CSharpCommandLineParser.Default, clientDirectory != null ? Path.Combine(clientDirectory, ResponseFileName) : null, args, clientDirectory, baseDirectory, sdkDirectory, libDirectory, analyzerLoader) { - _compilerServerHost = compilerServerHost; + _metadataProvider = metadataProvider; } internal override Func GetMetadataProvider() { - return _compilerServerHost.AssemblyReferenceProvider; + return _metadataProvider; } protected override uint GetSqmAppID() diff --git a/src/Compilers/Server/ServerShared/CompilerRequestHandler.cs b/src/Compilers/Server/ServerShared/CompilerRequestHandler.cs index ec58a9b2b9c..814a64feeda 100644 --- a/src/Compilers/Server/ServerShared/CompilerRequestHandler.cs +++ b/src/Compilers/Server/ServerShared/CompilerRequestHandler.cs @@ -11,6 +11,8 @@ using System.Threading.Tasks; using Microsoft.CodeAnalysis.CommandLine; +using static Microsoft.CodeAnalysis.CommandLine.CompilerServerLogger; + namespace Microsoft.CodeAnalysis.CompilerServer { internal struct RunRequest @@ -59,7 +61,7 @@ public bool TryCreateCompiler(RunRequest request, out CommonCompiler compiler) { case LanguageNames.CSharp: compiler = new CSharpCompilerServer( - this, + AssemblyReferenceProvider, args: request.Arguments, clientDirectory: ClientDirectory, baseDirectory: request.CurrentDirectory, @@ -69,7 +71,7 @@ public bool TryCreateCompiler(RunRequest request, out CommonCompiler compiler) return true; case LanguageNames.VisualBasic: compiler = new VisualBasicCompilerServer( - this, + AssemblyReferenceProvider, args: request.Arguments, clientDirectory: ClientDirectory, baseDirectory: request.CurrentDirectory, @@ -82,5 +84,39 @@ public bool TryCreateCompiler(RunRequest request, out CommonCompiler compiler) return false; } } + + public BuildResponse RunCompilation(RunRequest request, CancellationToken cancellationToken) + { + Log($"CurrentDirectory = '{request.CurrentDirectory}'"); + Log($"LIB = '{request.LibDirectory}'"); + for (int i = 0; i < request.Arguments.Length; ++i) + { + Log($"Argument[{i}] = '{request.Arguments[i]}'"); + } + + CommonCompiler compiler; + if (!TryCreateCompiler(request, out compiler)) + { + // TODO: is this the right option? Right now it will cause the fall back in the command line + // to fail because it's a valid response. Probably should add a "server failed" response + // for command line to deal with. + + // We can't do anything with a request we don't know about. + Log($"Got request with id '{request.Language}'"); + return new CompletedBuildResponse(returnCode: -1, utf8output: false, output: "", errorOutput: ""); + } + + bool utf8output = compiler.Arguments.Utf8Output; + if (!CheckAnalyzers(request.CurrentDirectory, compiler.Arguments.AnalyzerReferences)) + { + return new AnalyzerInconsistencyBuildResponse(); + } + + Log($"****Running {request.Language} compiler..."); + TextWriter output = new StringWriter(CultureInfo.InvariantCulture); + int returnCode = compiler.Run(output, cancellationToken); + Log($"****{request.Language} Compilation complete.\r\n****Return code: {returnCode}\r\n****Output:\r\n{output.ToString()}\r\n"); + return new CompletedBuildResponse(returnCode, utf8output, output.ToString(), ""); + } } } diff --git a/src/Compilers/Server/ServerShared/Connection.cs b/src/Compilers/Server/ServerShared/Connection.cs index ff4f84c3b2d..5bb194d60e2 100644 --- a/src/Compilers/Server/ServerShared/Connection.cs +++ b/src/Compilers/Server/ServerShared/Connection.cs @@ -200,15 +200,18 @@ private bool IsShutdownRequest(BuildRequest request) return request.Arguments.Length == 1 && request.Arguments[0].ArgumentId == BuildProtocolConstants.ArgumentId.Shutdown; } - protected virtual Task ServeBuildRequest(BuildRequest request, CancellationToken cancellationToken) + protected virtual Task ServeBuildRequest(BuildRequest buildRequest, CancellationToken cancellationToken) { - return Task.Run(() => + Func func = () => { try { // Do the compilation Log("Begin compilation"); - BuildResponse response = ServeBuildRequestCore(request, cancellationToken); + + var request = BuildProtocolUtil.GetRunRequest(buildRequest); + var response = _compilerServerHost.RunCompilation(request, cancellationToken); + Log("End compilation"); return response; } @@ -216,42 +219,11 @@ protected virtual Task ServeBuildRequest(BuildRequest request, Ca { throw ExceptionUtilities.Unreachable; } - }); - } - - private BuildResponse ServeBuildRequestCore(BuildRequest buildRequest, CancellationToken cancellationToken) - { - var request = BuildProtocolUtil.GetRunRequest(buildRequest); - CommonCompiler compiler; - if (!_compilerServerHost.TryCreateCompiler(request, out compiler)) - { - // TODO: is this the right option? Right now it will cause the fall back in the command line - // to fail because it's a valid response. Probably should add a "server failed" response - // for command line to deal with. - - // We can't do anything with a request we don't know about. - Log($"Got request with id '{request.Language}'"); - return new CompletedBuildResponse(-1, false, "", ""); - } - - Log($"CurrentDirectory = '{request.CurrentDirectory}'"); - Log($"LIB = '{request.LibDirectory}'"); - for (int i = 0; i < request.Arguments.Length; ++i) - { - Log($"Argument[{i}] = '{request.Arguments[i]}'"); - } - - bool utf8output = compiler.Arguments.Utf8Output; - if (!_compilerServerHost.CheckAnalyzers(request.CurrentDirectory, compiler.Arguments.AnalyzerReferences)) - { - return new AnalyzerInconsistencyBuildResponse(); - } + }; - Log($"****Running {request.Language} compiler..."); - TextWriter output = new StringWriter(CultureInfo.InvariantCulture); - int returnCode = compiler.Run(output, cancellationToken); - Log($"****{request.Language} Compilation complete.\r\n****Return code: {returnCode}\r\n****Output:\r\n{output.ToString()}\r\n"); - return new CompletedBuildResponse(returnCode, utf8output, output.ToString(), ""); + var task = new Task(func, cancellationToken, TaskCreationOptions.LongRunning); + task.Start(); + return task; } private void Log(string message) diff --git a/src/Compilers/Server/ServerShared/ICompilerServerHost.cs b/src/Compilers/Server/ServerShared/ICompilerServerHost.cs index 19ed8b29454..6f67127b782 100644 --- a/src/Compilers/Server/ServerShared/ICompilerServerHost.cs +++ b/src/Compilers/Server/ServerShared/ICompilerServerHost.cs @@ -7,13 +7,12 @@ using System.Text; using System.Threading; using System.Threading.Tasks; +using Microsoft.CodeAnalysis.CommandLine; namespace Microsoft.CodeAnalysis.CompilerServer { internal interface ICompilerServerHost { - Func AssemblyReferenceProvider { get; } - bool TryCreateCompiler(RunRequest request, out CommonCompiler compiler); - bool CheckAnalyzers(string baseDirectory, ImmutableArray analyzers); + BuildResponse RunCompilation(RunRequest request, CancellationToken cancellationToken); } } diff --git a/src/Compilers/Server/ServerShared/VisualBasicCompilerServer.cs b/src/Compilers/Server/ServerShared/VisualBasicCompilerServer.cs index 7e51fd18101..bd52bc7ec10 100644 --- a/src/Compilers/Server/ServerShared/VisualBasicCompilerServer.cs +++ b/src/Compilers/Server/ServerShared/VisualBasicCompilerServer.cs @@ -15,17 +15,17 @@ namespace Microsoft.CodeAnalysis.CompilerServer { internal sealed class VisualBasicCompilerServer : VisualBasicCompiler { - private readonly ICompilerServerHost _compilerServerHost; + private readonly Func _metadataProvider; - internal VisualBasicCompilerServer(ICompilerServerHost compilerServerHost, string[] args, string clientDirectory, string baseDirectory, string sdkDirectory, string libDirectory, IAnalyzerAssemblyLoader analyzerLoader) + internal VisualBasicCompilerServer(Func metadataProvider, string[] args, string clientDirectory, string baseDirectory, string sdkDirectory, string libDirectory, IAnalyzerAssemblyLoader analyzerLoader) : base(VisualBasicCommandLineParser.Default, clientDirectory != null ? Path.Combine(clientDirectory, ResponseFileName) : null, args, clientDirectory, baseDirectory, sdkDirectory, libDirectory, analyzerLoader) { - _compilerServerHost = compilerServerHost; + _metadataProvider = metadataProvider; } internal override Func GetMetadataProvider() { - return _compilerServerHost.AssemblyReferenceProvider; + return _metadataProvider; } protected override uint GetSqmAppID() diff --git a/src/Compilers/Server/VBCSCompiler/DesktopCompilerServerHost.cs b/src/Compilers/Server/VBCSCompiler/DesktopCompilerServerHost.cs index 2459a2d499d..648bb2d8e46 100644 --- a/src/Compilers/Server/VBCSCompiler/DesktopCompilerServerHost.cs +++ b/src/Compilers/Server/VBCSCompiler/DesktopCompilerServerHost.cs @@ -21,11 +21,11 @@ internal sealed class DesktopCompilerServerHost : CompilerServerHost private static readonly IAnalyzerAssemblyLoader s_analyzerLoader = new ShadowCopyAnalyzerAssemblyLoader(Path.Combine(Path.GetTempPath(), "VBCSCompiler", "AnalyzerAssemblyLoader")); // Caches are used by C# and VB compilers, and shared here. - private static readonly Func s_assemblyReferenceProvider = (path, properties) => new CachingMetadataReference(path, properties); + public static readonly Func SharedAssemblyReferenceProvider = (path, properties) => new CachingMetadataReference(path, properties); public override IAnalyzerAssemblyLoader AnalyzerAssemblyLoader => s_analyzerLoader; - public override Func AssemblyReferenceProvider => s_assemblyReferenceProvider; + public override Func AssemblyReferenceProvider => SharedAssemblyReferenceProvider; internal DesktopCompilerServerHost() : this(AppDomain.CurrentDomain.BaseDirectory, RuntimeEnvironment.GetRuntimeDirectory()) diff --git a/src/Compilers/Server/VBCSCompilerTests/CompilerServerApiTest.cs b/src/Compilers/Server/VBCSCompilerTests/CompilerServerApiTest.cs index 85dc47947f7..4920f2aa21c 100644 --- a/src/Compilers/Server/VBCSCompilerTests/CompilerServerApiTest.cs +++ b/src/Compilers/Server/VBCSCompilerTests/CompilerServerApiTest.cs @@ -11,6 +11,7 @@ using Roslyn.Test.Utilities; using Xunit; using System.Runtime.InteropServices; +using System.Diagnostics; namespace Microsoft.CodeAnalysis.CompilerServer.UnitTests { @@ -96,6 +97,13 @@ private async Task CreateBuildRequest(string sourceText, TimeSpan? builder.ToImmutable()); } + private static async Task Verify(ServerData serverData, int connections, int completed) + { + var serverStats = await serverData.Complete().ConfigureAwait(true); + Assert.Equal(connections, serverStats.Connections); + Assert.Equal(completed, serverStats.CompletedConnections); + } + /// /// Run a C# compilation against the given source text using the provided named pipe name. /// @@ -353,5 +361,54 @@ public void MutexAcquiredWhenRunningServer() var result = VBCSCompiler.Run(mutexName, host.Object, keepAlive: TimeSpan.FromSeconds(1)); Assert.Equal(CommonCompiler.Succeeded, result); } + + [Fact] + public async Task ShutdownRequestDirect() + { + using (var serverData = ServerUtil.CreateServer()) + { + var serverProcessId = await ServerUtil.SendShutdown(serverData.PipeName); + Assert.Equal(Process.GetCurrentProcess().Id, serverProcessId); + await Verify(serverData, connections: 1, completed: 1); + } + } + + /// + /// A shutdown request should not abort an existing compilation. It should be allowed to run to + /// completion. + /// + [Fact] + public async Task ShutdownDoesNotAbortCompilation() + { + var host = new TestableCompilerServerHost(); + + using (var startedMre = new ManualResetEvent(initialState: false)) + using (var finishedMre = new ManualResetEvent(initialState: false)) + using (var serverData = ServerUtil.CreateServer(compilerServerHost: host)) + { + // Create a compilation that is guaranteed to complete after the shutdown is seen. + host.RunCompilation = (request, cancellationToken) => + { + startedMre.Set(); + finishedMre.WaitOne(); + return s_emptyBuildResponse; + }; + + var compileTask = ServerUtil.Send(serverData.PipeName, s_emptyCSharpBuildRequest); + startedMre.WaitOne(); + + // The compilation is now in progress, send the shutdown. + await ServerUtil.SendShutdown(serverData.PipeName); + Assert.False(compileTask.IsCompleted); + finishedMre.Set(); + + var response = await compileTask; + Assert.Equal(BuildResponse.ResponseType.Completed, response.Type); + Assert.Equal(0, ((CompletedBuildResponse)response).ReturnCode); + + await Verify(serverData, connections: 2, completed: 2); + } + } + } } diff --git a/src/Compilers/Server/VBCSCompilerTests/CompilerServerTests.cs b/src/Compilers/Server/VBCSCompilerTests/CompilerServerTests.cs index ecb0620ea85..2bd0e23d0ad 100644 --- a/src/Compilers/Server/VBCSCompilerTests/CompilerServerTests.cs +++ b/src/Compilers/Server/VBCSCompilerTests/CompilerServerTests.cs @@ -1409,26 +1409,6 @@ public void BadKeepAlive4() Assert.Equal("", result.Errors); } - [Fact] - public async Task ShutdownRequestDirect() - { - using (var serverData = ServerUtil.CreateServer()) - using (var client = new NamedPipeClientStream(serverData.PipeName)) - { - await client.ConnectAsync(); - - var memoryStream = new MemoryStream(); - await BuildRequest.CreateShutdown().WriteAsync(memoryStream); - memoryStream.Position = 0; - await memoryStream.CopyToAsync(client); - - var response = await BuildResponse.ReadAsync(client); - Assert.Equal(BuildResponse.ResponseType.Shutdown, response.Type); - Assert.Equal(Process.GetCurrentProcess().Id, ((ShutdownBuildResponse)response).ServerProcessId); - await Verify(serverData, connections: 1, completed: 1); - } - } - [Fact] [WorkItem(1024619, "DevDiv")] public async Task Bug1024619_01() diff --git a/src/Compilers/Server/VBCSCompilerTests/ServerUtil.cs b/src/Compilers/Server/VBCSCompilerTests/ServerUtil.cs index 423f39e6f02..cbe108e20f6 100644 --- a/src/Compilers/Server/VBCSCompilerTests/ServerUtil.cs +++ b/src/Compilers/Server/VBCSCompilerTests/ServerUtil.cs @@ -144,6 +144,27 @@ internal static ServerData CreateServerFailsConnection(string pipeName = null) return new ServerData(cts, taskSource.Task, pipeName); } + internal static async Task Send(string pipeName, BuildRequest request) + { + using (var client = new NamedPipeClientStream(pipeName)) + { + await client.ConnectAsync(); + + var memoryStream = new MemoryStream(); + await request.WriteAsync(memoryStream); + memoryStream.Position = 0; + await memoryStream.CopyToAsync(client); + + return await BuildResponse.ReadAsync(client); + } + } + + internal static async Task SendShutdown(string pipeName) + { + var response = await Send(pipeName, BuildRequest.CreateShutdown()); + return ((ShutdownBuildResponse)response).ServerProcessId; + } + private static async Task CreateServerFailsConnectionCore(string pipeName, CancellationToken cancellationToken) { var connections = 0; diff --git a/src/Compilers/Server/VBCSCompilerTests/TestableCompilerServerHost.cs b/src/Compilers/Server/VBCSCompilerTests/TestableCompilerServerHost.cs new file mode 100644 index 00000000000..3f2b8788700 --- /dev/null +++ b/src/Compilers/Server/VBCSCompilerTests/TestableCompilerServerHost.cs @@ -0,0 +1,23 @@ +// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.CodeAnalysis.CommandLine; +using System; +using System.Threading; + +namespace Microsoft.CodeAnalysis.CompilerServer.UnitTests +{ + internal sealed class TestableCompilerServerHost : ICompilerServerHost + { + internal Func RunCompilation; + + internal TestableCompilerServerHost(Func runCompilation = null) + { + RunCompilation = runCompilation; + } + + BuildResponse ICompilerServerHost.RunCompilation(RunRequest request, CancellationToken cancellationToken) + { + return RunCompilation(request, cancellationToken); + } + } +} diff --git a/src/Compilers/Server/VBCSCompilerTests/VBCSCompilerTests.csproj b/src/Compilers/Server/VBCSCompilerTests/VBCSCompilerTests.csproj index 63866189222..5ce90f61215 100644 --- a/src/Compilers/Server/VBCSCompilerTests/VBCSCompilerTests.csproj +++ b/src/Compilers/Server/VBCSCompilerTests/VBCSCompilerTests.csproj @@ -117,6 +117,7 @@ + diff --git a/src/Compilers/VisualBasic/Test/CommandLine/TouchedFileLoggingTests.vb b/src/Compilers/VisualBasic/Test/CommandLine/TouchedFileLoggingTests.vb index 4019c0802ac..f1e287646cc 100644 --- a/src/Compilers/VisualBasic/Test/CommandLine/TouchedFileLoggingTests.vb +++ b/src/Compilers/VisualBasic/Test/CommandLine/TouchedFileLoggingTests.vb @@ -169,7 +169,7 @@ End Class Dim outWriter = New StringWriter() Dim cmd = New VisualBasicCompilerServer( - New DesktopCompilerServerHost(), + DesktopCompilerServerHost.SharedAssemblyReferenceProvider, {"/nologo", "/touchedfiles:" + touchedBase, source1}, -- GitLab