From bf171a6d68ebbf6a9a0dd4bbb228e421ebe0b9ea Mon Sep 17 00:00:00 2001 From: Jason Malinowski Date: Thu, 15 May 2025 12:17:59 -0700 Subject: [PATCH 1/2] Ensure we pass unique binlog paths to each BuildHost Otherwise if we launch multiple processes, they might step atop each other and cause locking issues. --- .../{BinlogNamer.cs => BinLogPathProvider.cs} | 11 ++++++----- .../FileBasedProgramsProjectSystem.cs | 4 ++-- .../FileBasedProgramsWorkspaceProviderFactory.cs | 5 +++-- .../HostWorkspace/LanguageServerProjectLoader.cs | 10 ++++------ .../HostWorkspace/LanguageServerProjectSystem.cs | 4 ++-- .../Core/MSBuild/BuildHostProcessManager.cs | 10 +++++----- .../MSBuild/Core/MSBuild/IBinLogPathProvider.cs | 15 +++++++++++++++ .../MSBuild/Test/BuildHostProcessManagerTests.cs | 6 +++++- 8 files changed, 42 insertions(+), 23 deletions(-) rename src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/{BinlogNamer.cs => BinLogPathProvider.cs} (83%) create mode 100644 src/Workspaces/MSBuild/Core/MSBuild/IBinLogPathProvider.cs diff --git a/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/BinlogNamer.cs b/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/BinLogPathProvider.cs similarity index 83% rename from src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/BinlogNamer.cs rename to src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/BinLogPathProvider.cs index c2535e2923d99..05958b1a0a796 100644 --- a/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/BinlogNamer.cs +++ b/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/BinLogPathProvider.cs @@ -4,14 +4,15 @@ using System.Composition; using Microsoft.CodeAnalysis.Host.Mef; +using Microsoft.CodeAnalysis.MSBuild; using Microsoft.CodeAnalysis.Options; using Microsoft.Extensions.Logging; using Microsoft.VisualStudio.Composition; namespace Microsoft.CodeAnalysis.LanguageServer.HostWorkspace; -[Export(typeof(BinlogNamer)), Shared] -internal sealed class BinlogNamer +[Export(typeof(IBinLogPathProvider)), Shared] +internal sealed class BinLogPathProvider : IBinLogPathProvider { /// /// The suffix to use for the binary log name; incremented each time we have a new build. Should be incremented with . @@ -28,13 +29,13 @@ internal sealed class BinlogNamer [ImportingConstructor] [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] - public BinlogNamer(IGlobalOptionService globalOptionService, ILoggerFactory loggerFactory) + public BinLogPathProvider(IGlobalOptionService globalOptionService, ILoggerFactory loggerFactory) { _globalOptionService = globalOptionService; - _logger = loggerFactory.CreateLogger(); + _logger = loggerFactory.CreateLogger(); } - internal string? GetMSBuildBinaryLogPath() + public string? GetNewLogPath() { if (_globalOptionService.GetOption(LanguageServerProjectSystemOptionsStorage.BinaryLogPath) is not string binaryLogDirectory) return null; diff --git a/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/FileBasedProgramsProjectSystem.cs b/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/FileBasedProgramsProjectSystem.cs index 4b229d8497aee..f60b31557eeef 100644 --- a/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/FileBasedProgramsProjectSystem.cs +++ b/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/FileBasedProgramsProjectSystem.cs @@ -41,7 +41,7 @@ public FileBasedProgramsProjectSystem( IAsynchronousOperationListenerProvider listenerProvider, ProjectLoadTelemetryReporter projectLoadTelemetry, ServerConfigurationFactory serverConfigurationFactory, - BinlogNamer binlogNamer) + IBinLogPathProvider binLogPathProvider) : base( workspaceFactory.FileBasedProgramsProjectFactory, workspaceFactory.TargetFrameworkManager, @@ -52,7 +52,7 @@ public FileBasedProgramsProjectSystem( listenerProvider, projectLoadTelemetry, serverConfigurationFactory, - binlogNamer) + binLogPathProvider) { _lspServices = lspServices; _logger = loggerFactory.CreateLogger(); diff --git a/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/FileBasedProgramsWorkspaceProviderFactory.cs b/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/FileBasedProgramsWorkspaceProviderFactory.cs index d7c63e9f2aed0..de90d3175e244 100644 --- a/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/FileBasedProgramsWorkspaceProviderFactory.cs +++ b/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/FileBasedProgramsWorkspaceProviderFactory.cs @@ -8,6 +8,7 @@ using Microsoft.CodeAnalysis.LanguageServer.Handler; using Microsoft.CodeAnalysis.LanguageServer.HostWorkspace.ProjectTelemetry; using Microsoft.CodeAnalysis.MetadataAsSource; +using Microsoft.CodeAnalysis.MSBuild; using Microsoft.CodeAnalysis.Options; using Microsoft.CodeAnalysis.ProjectSystem; using Microsoft.CodeAnalysis.Shared.TestHooks; @@ -33,10 +34,10 @@ internal sealed class FileBasedProgramsWorkspaceProviderFactory( IAsynchronousOperationListenerProvider listenerProvider, ProjectLoadTelemetryReporter projectLoadTelemetry, ServerConfigurationFactory serverConfigurationFactory, - BinlogNamer binlogNamer) : ILspMiscellaneousFilesWorkspaceProviderFactory + IBinLogPathProvider binLogPathProvider) : ILspMiscellaneousFilesWorkspaceProviderFactory { public ILspMiscellaneousFilesWorkspaceProvider CreateLspMiscellaneousFilesWorkspaceProvider(ILspServices lspServices, HostServices hostServices) { - return new FileBasedProgramsProjectSystem(lspServices, metadataAsSourceFileService, workspaceFactory, fileChangeWatcher, globalOptionService, loggerFactory, listenerProvider, projectLoadTelemetry, serverConfigurationFactory, binlogNamer); + return new FileBasedProgramsProjectSystem(lspServices, metadataAsSourceFileService, workspaceFactory, fileChangeWatcher, globalOptionService, loggerFactory, listenerProvider, projectLoadTelemetry, serverConfigurationFactory, binLogPathProvider); } } diff --git a/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectLoader.cs b/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectLoader.cs index d97745c2edb92..b16e6c6ccf0db 100644 --- a/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectLoader.cs +++ b/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectLoader.cs @@ -43,7 +43,7 @@ internal abstract class LanguageServerProjectLoader protected readonly ILoggerFactory LoggerFactory; private readonly ILogger _logger; private readonly ProjectLoadTelemetryReporter _projectLoadTelemetryReporter; - private readonly BinlogNamer _binlogNamer; + private readonly IBinLogPathProvider _binLogPathProvider; protected readonly ImmutableDictionary AdditionalProperties; /// @@ -98,7 +98,7 @@ protected LanguageServerProjectLoader( IAsynchronousOperationListenerProvider listenerProvider, ProjectLoadTelemetryReporter projectLoadTelemetry, ServerConfigurationFactory serverConfigurationFactory, - BinlogNamer binlogNamer) + IBinLogPathProvider binLogPathProvider) { ProjectFactory = projectFactory; _targetFrameworkManager = targetFrameworkManager; @@ -108,7 +108,7 @@ protected LanguageServerProjectLoader( LoggerFactory = loggerFactory; _logger = loggerFactory.CreateLogger(nameof(LanguageServerProjectLoader)); _projectLoadTelemetryReporter = projectLoadTelemetry; - _binlogNamer = binlogNamer; + _binLogPathProvider = binLogPathProvider; var workspace = projectFactory.Workspace; var razorDesignTimePath = serverConfigurationFactory.ServerConfiguration?.RazorDesignTimePath; @@ -145,9 +145,7 @@ private async ValueTask ReloadProjectsAsync(ImmutableSegmentedList _globalMSBuildProperties; private readonly ILoggerFactory? _loggerFactory; private readonly ILogger? _logger; - private readonly string? _binaryLogPath; + private readonly IBinLogPathProvider? _binaryLogPathProvider; private readonly SemaphoreSlim _gate = new(initialCount: 1); private readonly Dictionary _processes = []; - public BuildHostProcessManager(ImmutableDictionary? globalMSBuildProperties = null, string? binaryLogPath = null, ILoggerFactory? loggerFactory = null) + public BuildHostProcessManager(ImmutableDictionary? globalMSBuildProperties = null, IBinLogPathProvider? binaryLogPathProvider = null, ILoggerFactory? loggerFactory = null) { _globalMSBuildProperties = globalMSBuildProperties ?? ImmutableDictionary.Empty; - _binaryLogPath = binaryLogPath; + _binaryLogPathProvider = binaryLogPathProvider; _loggerFactory = loggerFactory; _logger = loggerFactory?.CreateLogger(); } @@ -244,10 +244,10 @@ private void AppendBuildHostCommandLineArgumentsConfigureProcess(ProcessStartInf AddArgument(processStartInfo, globalMSBuildProperty.Key + '=' + globalMSBuildProperty.Value); } - if (_binaryLogPath is not null) + if (_binaryLogPathProvider?.GetNewLogPath() is string binaryLogPath) { AddArgument(processStartInfo, "--binlog"); - AddArgument(processStartInfo, _binaryLogPath); + AddArgument(processStartInfo, binaryLogPath); } AddArgument(processStartInfo, "--locale"); diff --git a/src/Workspaces/MSBuild/Core/MSBuild/IBinLogPathProvider.cs b/src/Workspaces/MSBuild/Core/MSBuild/IBinLogPathProvider.cs new file mode 100644 index 0000000000000..d2a47962f4d37 --- /dev/null +++ b/src/Workspaces/MSBuild/Core/MSBuild/IBinLogPathProvider.cs @@ -0,0 +1,15 @@ +// 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. + +namespace Microsoft.CodeAnalysis.MSBuild; + +internal interface IBinLogPathProvider +{ + /// + /// Returns a new log path. Each call will return a new name, so that way we don't have collisions if multiple processes are writing to different logs. + /// + /// A new path, or null if no logging is currently wanted. An instance is allowed to switch between return null and returning non-null if + /// the user changes configuration. + string? GetNewLogPath(); +} diff --git a/src/Workspaces/MSBuild/Test/BuildHostProcessManagerTests.cs b/src/Workspaces/MSBuild/Test/BuildHostProcessManagerTests.cs index 8db19e288e2cc..c89105505db99 100644 --- a/src/Workspaces/MSBuild/Test/BuildHostProcessManagerTests.cs +++ b/src/Workspaces/MSBuild/Test/BuildHostProcessManagerTests.cs @@ -4,6 +4,7 @@ using System.Collections.Generic; using System.Collections.Immutable; +using Moq; using Roslyn.Test.Utilities; using Xunit; @@ -65,7 +66,10 @@ internal void ProcessStartInfo_PassesBinLogPath(BuildHostProcessKind buildHostKi { const string BinaryLogPath = "test.binlog"; - var processStartInfo = new BuildHostProcessManager(binaryLogPath: BinaryLogPath) + var binLogPathProviderMock = new Mock(MockBehavior.Strict); + binLogPathProviderMock.Setup(m => m.GetNewLogPath()).Returns(BinaryLogPath); + + var processStartInfo = new BuildHostProcessManager(binaryLogPathProvider: binLogPathProviderMock.Object) .CreateBuildHostStartInfo(buildHostKind, pipeName: ""); #if NET From 12369bc2568fa148fe2868d817262f090277621d Mon Sep 17 00:00:00 2001 From: Jason Malinowski Date: Thu, 15 May 2025 12:47:56 -0700 Subject: [PATCH 2/2] Clean up our SpellingExclusions Somehow almost every line started with a UTF-8 BOM. This fixes it. --- SpellingExclusions.dic | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/SpellingExclusions.dic b/SpellingExclusions.dic index 3d49d6e1f8040..6bcf960f73faf 100644 --- a/SpellingExclusions.dic +++ b/SpellingExclusions.dic @@ -1,7 +1,6 @@ -stackalloc +stackalloc awaitable -Refactorings -Infos -cref -binlog -Namer +Refactorings +Infos +cref +binlog \ No newline at end of file