-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Run design time builds out-of-process #69616
Changes from all commits
a827ef9
53ee4ec
a6e3cfb
64d345b
ac72bba
71b655c
5c7a3db
e93ef80
0fba304
abd9885
79f8561
1a419e1
f9621a1
22bbff7
2435e4f
b4b64d1
df13de5
e4ec102
51e6e45
1c53ab7
4a152e8
e919bf4
88f5c21
66ac5d9
75f2235
022c233
b2cc3b8
5e24433
df96112
ce7780c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
awaitable | ||
Refactorings | ||
Infos |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,269 @@ | ||
// 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. | ||
|
||
using System.Diagnostics; | ||
using System.Runtime.InteropServices; | ||
using System.Xml; | ||
using System.Xml.Linq; | ||
using Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost; | ||
using Microsoft.Extensions.Logging; | ||
using Roslyn.Utilities; | ||
using StreamJsonRpc; | ||
|
||
namespace Microsoft.CodeAnalysis.LanguageServer.HostWorkspace; | ||
|
||
internal sealed class BuildHostProcessManager : IAsyncDisposable | ||
{ | ||
private readonly ILoggerFactory? _loggerFactory; | ||
private readonly ILogger? _logger; | ||
private readonly string? _binaryLogPath; | ||
|
||
private readonly SemaphoreSlim _gate = new(initialCount: 1); | ||
private readonly Dictionary<BuildHostProcessKind, BuildHostProcess> _processes = new(); | ||
|
||
public BuildHostProcessManager(ILoggerFactory? loggerFactory = null, string? binaryLogPath = null) | ||
{ | ||
_loggerFactory = loggerFactory; | ||
_logger = loggerFactory?.CreateLogger<BuildHostProcessManager>(); | ||
_binaryLogPath = binaryLogPath; | ||
} | ||
|
||
public async Task<IBuildHost> GetBuildHostAsync(string projectFilePath, CancellationToken cancellationToken) | ||
{ | ||
var neededBuildHostKind = GetKindForProject(projectFilePath); | ||
|
||
_logger?.LogTrace($"Choosing a build host of type {neededBuildHostKind} for {projectFilePath}"); | ||
|
||
using (await _gate.DisposableWaitAsync(cancellationToken).ConfigureAwait(false)) | ||
{ | ||
if (!_processes.TryGetValue(neededBuildHostKind, out var buildHostProcess)) | ||
{ | ||
var process = neededBuildHostKind switch | ||
{ | ||
BuildHostProcessKind.NetCore => LaunchDotNetCoreBuildHost(), | ||
BuildHostProcessKind.NetFramework => LaunchDotNetFrameworkBuildHost(), | ||
_ => throw ExceptionUtilities.UnexpectedValue(neededBuildHostKind) | ||
}; | ||
|
||
buildHostProcess = new BuildHostProcess(process, _loggerFactory); | ||
buildHostProcess.Disconnected += BuildHostProcess_Disconnected; | ||
_processes.Add(neededBuildHostKind, buildHostProcess); | ||
} | ||
|
||
return buildHostProcess.BuildHost; | ||
} | ||
} | ||
|
||
private void BuildHostProcess_Disconnected(object? sender, EventArgs e) | ||
{ | ||
Contract.ThrowIfNull(sender, $"{nameof(BuildHostProcess)}.{nameof(BuildHostProcess.Disconnected)} was raised with a null sender."); | ||
jasonmalinowski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Task.Run(async () => | ||
{ | ||
BuildHostProcess? processToDispose = null; | ||
|
||
using (await _gate.DisposableWaitAsync().ConfigureAwait(false)) | ||
{ | ||
// Remove it from our map; it's possible it might have already been removed if we had more than one way we observed a disconnect. | ||
var existingProcess = _processes.SingleOrNull(p => p.Value == sender); | ||
if (existingProcess.HasValue) | ||
{ | ||
processToDispose = existingProcess.Value.Value; | ||
_processes.Remove(existingProcess.Value.Key); | ||
} | ||
} | ||
|
||
// Dispose outside of the lock (even though we don't expect much to happen at this point) | ||
if (processToDispose != null) | ||
{ | ||
processToDispose.LoggerForProcessMessages?.LogTrace("Process exited."); | ||
await processToDispose.DisposeAsync(); | ||
} | ||
}); | ||
} | ||
|
||
public async ValueTask DisposeAsync() | ||
{ | ||
foreach (var process in _processes.Values) | ||
await process.DisposeAsync(); | ||
} | ||
|
||
private Process LaunchDotNetCoreBuildHost() | ||
{ | ||
var processStartInfo = new ProcessStartInfo() | ||
{ | ||
FileName = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "dotnet.exe" : "dotnet", | ||
}; | ||
|
||
// We need to roll forward to the latest runtime, since the project may be using an SDK (or an SDK required runtime) newer than we ourselves built with. | ||
// We set the environment variable since --roll-forward LatestMajor doesn't roll forward to prerelease SDKs otherwise. | ||
processStartInfo.Environment["DOTNET_ROLL_FORWARD_TO_PRERELEASE"] = "1"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So what we're doing here is launching the build host with whatever the latest major version they have installed is right? What directory are we launching this from - I think that could affect which dotnet we use if they for example have a global.json? If it does matter, we should potentially be explicit about what the working directory should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good questions!
At the moment, yes, since MSBuildLocator currently requires that you have to be running the same or higher major version to even discover the applicable SDK in the first place; it's an annoying catch-22. And once that process is launched, we will find the global.json for the first project we then ask it to process when we invoke MSBuildLocator there. Once this is in, I plan on making a change to MSBuildLocator that would allow us to discover (but not register) the applicable SDK version directly. In that case:
This is still a reminder for me though that was brought up that DevKit might explicitly set a working directory so there's no risk this locks a user folder underneath us. |
||
processStartInfo.ArgumentList.Add("--roll-forward"); | ||
processStartInfo.ArgumentList.Add("LatestMajor"); | ||
|
||
processStartInfo.ArgumentList.Add(typeof(IBuildHost).Assembly.Location); | ||
|
||
AppendBuildHostCommandLineArgumentsConfigureProcess(processStartInfo); | ||
|
||
var process = Process.Start(processStartInfo); | ||
Contract.ThrowIfNull(process, "Process.Start failed to launch a process."); | ||
return process; | ||
} | ||
|
||
private Process LaunchDotNetFrameworkBuildHost() | ||
{ | ||
var netFrameworkBuildHost = Path.Combine(Path.GetDirectoryName(typeof(BuildHostProcessManager).Assembly.Location)!, "BuildHost-net472", "Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.exe"); | ||
Contract.ThrowIfFalse(File.Exists(netFrameworkBuildHost), $"Unable to locate the .NET Framework build host at {netFrameworkBuildHost}"); | ||
|
||
var processStartInfo = new ProcessStartInfo() | ||
{ | ||
FileName = netFrameworkBuildHost, | ||
}; | ||
|
||
AppendBuildHostCommandLineArgumentsConfigureProcess(processStartInfo); | ||
|
||
var process = Process.Start(processStartInfo); | ||
Contract.ThrowIfNull(process, "Process.Start failed to launch a process."); | ||
return process; | ||
} | ||
|
||
private void AppendBuildHostCommandLineArgumentsConfigureProcess(ProcessStartInfo processStartInfo) | ||
{ | ||
if (_binaryLogPath is not null) | ||
{ | ||
processStartInfo.ArgumentList.Add("--binlog"); | ||
processStartInfo.ArgumentList.Add(_binaryLogPath); | ||
} | ||
|
||
// MSBUILD_EXE_PATH is read by MSBuild to find related tasks and targets. We don't want this to be inherited by our build process, or otherwise | ||
// it might try to load targets that aren't appropriate for the build host. | ||
processStartInfo.Environment.Remove("MSBUILD_EXE_PATH"); | ||
|
||
processStartInfo.CreateNoWindow = true; | ||
processStartInfo.UseShellExecute = false; | ||
processStartInfo.RedirectStandardInput = true; | ||
processStartInfo.RedirectStandardOutput = true; | ||
processStartInfo.RedirectStandardError = true; | ||
} | ||
|
||
private static readonly XmlReaderSettings s_xmlSettings = new() | ||
{ | ||
DtdProcessing = DtdProcessing.Prohibit, | ||
}; | ||
|
||
private static BuildHostProcessKind GetKindForProject(string projectFilePath) | ||
{ | ||
// At the moment we don't have mono support here, so if we're not on Windows, we'll always force to .NET Core. | ||
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
return BuildHostProcessKind.NetCore; | ||
|
||
// This implements the algorithm as stated in https://github.com/dotnet/project-system/blob/9a761848e0f330a45e349685a266fea00ac3d9c5/docs/opening-with-new-project-system.md; | ||
// we'll load the XML of the project directly, and inspect for certain elements. | ||
XDocument document; | ||
|
||
// Read the XML, prohibiting DTD processing due the the usual concerns there. | ||
using (var fileStream = new FileStream(projectFilePath, FileMode.Open, FileAccess.Read)) | ||
using (var xmlReader = XmlReader.Create(fileStream, s_xmlSettings)) | ||
document = XDocument.Load(xmlReader); | ||
|
||
// If we don't have a root, doesn't really matter which. This project is just malformed. | ||
if (document.Root == null) | ||
return BuildHostProcessKind.NetCore; | ||
|
||
// Look for SDK attribute on the root | ||
if (document.Root.Attribute("Sdk") != null) | ||
return BuildHostProcessKind.NetCore; | ||
|
||
// Look for <Import Sdk=... /> | ||
if (document.Root.Elements("Import").Attributes("Sdk").Any()) | ||
return BuildHostProcessKind.NetCore; | ||
|
||
// Look for <Sdk ... /> | ||
if (document.Root.Elements("Sdk").Any()) | ||
return BuildHostProcessKind.NetCore; | ||
|
||
// Looking for PropertyGroups that contain TargetFramework or TargetFrameworks nodes | ||
var propertyGroups = document.Descendants("PropertyGroup"); | ||
if (propertyGroups.Elements("TargetFramework").Any() || propertyGroups.Elements("TargetFrameworks").Any()) | ||
return BuildHostProcessKind.NetCore; | ||
|
||
// Nothing that indicates it's an SDK-style project, so use our .NET framework host | ||
return BuildHostProcessKind.NetFramework; | ||
jasonmalinowski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
private enum BuildHostProcessKind | ||
{ | ||
NetCore, | ||
NetFramework | ||
} | ||
|
||
private sealed class BuildHostProcess : IAsyncDisposable | ||
{ | ||
private readonly Process _process; | ||
private readonly JsonRpc _jsonRpc; | ||
|
||
private int _disposed = 0; | ||
|
||
public BuildHostProcess(Process process, ILoggerFactory? loggerFactory) | ||
{ | ||
LoggerForProcessMessages = loggerFactory?.CreateLogger($"BuildHost PID {process.Id}"); | ||
|
||
_process = process; | ||
|
||
_process.EnableRaisingEvents = true; | ||
_process.Exited += Process_Exited; | ||
|
||
_process.ErrorDataReceived += Process_ErrorDataReceived; | ||
|
||
var messageHandler = new HeaderDelimitedMessageHandler(sendingStream: _process.StandardInput.BaseStream, receivingStream: _process.StandardOutput.BaseStream, new JsonMessageFormatter()); | ||
|
||
_jsonRpc = new JsonRpc(messageHandler); | ||
_jsonRpc.StartListening(); | ||
BuildHost = _jsonRpc.Attach<IBuildHost>(); | ||
|
||
// Call this last so our type is fully constructed before we start firing events | ||
_process.BeginErrorReadLine(); | ||
} | ||
|
||
private void Process_Exited(object? sender, EventArgs e) | ||
{ | ||
Disconnected?.Invoke(this, EventArgs.Empty); | ||
jasonmalinowski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
private void Process_ErrorDataReceived(object sender, DataReceivedEventArgs e) | ||
{ | ||
if (e.Data is not null) | ||
LoggerForProcessMessages?.LogTrace($"Message from Process: {e.Data}"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something to consider for later - similar to LSP trace logging, sometimes the json itself is useful to log at the most verbose levels. In case there are weird serialization issues. |
||
} | ||
|
||
public IBuildHost BuildHost { get; } | ||
public ILogger? LoggerForProcessMessages { get; } | ||
|
||
public event EventHandler? Disconnected; | ||
|
||
public async ValueTask DisposeAsync() | ||
{ | ||
// Ensure only one thing disposes; while we disconnect the process will go away, which will call us to do this again | ||
if (Interlocked.CompareExchange(ref _disposed, value: 1, comparand: 0) != 0) | ||
return; | ||
|
||
// We will call Shutdown in a try/catch; if the process has gone bad it's possible the connection is no longer functioning. | ||
try | ||
{ | ||
await BuildHost.ShutdownAsync(); | ||
_jsonRpc.Dispose(); | ||
|
||
LoggerForProcessMessages?.LogTrace("Process gracefully shut down."); | ||
} | ||
catch (Exception e) | ||
{ | ||
LoggerForProcessMessages?.LogError(e, "Exception while shutting down the BuildHost process."); | ||
|
||
// OK, process may have gone bad. | ||
_process.Kill(); | ||
} | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah interesting - so eventually we could even have different .net core build hosts at the same time if we needed (for different projects)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! DevKit doesn't support it, and it's not clear we should either from a user-scenario standpoint, but once we have MSBuildLocator behavior changed it's easier to write it right than maintain the current behavior. This type will eventually move to MSBuildWorkspace too where people might be doing programmatic things across multiple repositories too, so since we're an API it's harder to say "we don't support it".