From adf5458bd0fe065bb535adffc9c8e85e38be9aab Mon Sep 17 00:00:00 2001 From: Forgind <12969783+Forgind@users.noreply.github.com> Date: Tue, 14 Jan 2025 13:50:41 -0800 Subject: [PATCH 1/5] Parallelize workload pack downloads --- .../install/FileBasedInstaller.cs | 117 +++++++++--------- 1 file changed, 61 insertions(+), 56 deletions(-) diff --git a/src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs b/src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs index 1585c5e33f1f..a1699f5ff710 100644 --- a/src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs +++ b/src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Collections.Concurrent; using System.Text.Json; using Microsoft.DotNet.Cli; using Microsoft.DotNet.Cli.NuGetPackageDownloader; @@ -140,74 +141,81 @@ public void InstallWorkloads(IEnumerable workloadIds, SdkFeatureBand public void InstallWorkloads(IEnumerable workloadIds, SdkFeatureBand sdkFeatureBand, ITransactionContext transactionContext, bool overwriteExistingPacks, DirectoryPath? offlineCache = null) { var packInfos = GetPacksInWorkloads(workloadIds); - + List packsToInstall = []; foreach (var packInfo in packInfos) + { + if (PackIsInstalled(packInfo) && !overwriteExistingPacks) + { + _reporter.WriteLine(string.Format(LocalizableStrings.WorkloadPackAlreadyInstalledMessage, packInfo.ResolvedPackageId, packInfo.Version)); + } + else + { + packsToInstall.Add(packInfo); + } + } + + var tempFilesToDelete = new ConcurrentBag(); + var packagePaths = packsToInstall.AsParallel().Select(packInfo => + { + if (offlineCache == null || !offlineCache.HasValue) + { + var packagePath = _nugetPackageDownloader + .DownloadPackageAsync(new PackageId(packInfo.ResolvedPackageId), + new NuGetVersion(packInfo.Version), + _packageSourceLocation).GetAwaiter().GetResult(); + tempFilesToDelete.Add(packagePath); + return (packagePath, packInfo); + } + else + { + _reporter.WriteLine(string.Format(LocalizableStrings.UsingCacheForPackInstall, packInfo.ResolvedPackageId, packInfo.Version, offlineCache)); + var packagePath = Path.Combine(offlineCache.Value.Value, $"{packInfo.ResolvedPackageId}.{packInfo.Version}.nupkg"); + if (!File.Exists(packagePath)) + { + throw new Exception(string.Format(LocalizableStrings.CacheMissingPackage, packInfo.ResolvedPackageId, packInfo.Version, offlineCache)); + } + return (packagePath, packInfo); + } + }); + + foreach (var (packagePath, packInfo) in packagePaths) { _reporter.WriteLine(string.Format(LocalizableStrings.InstallingPackVersionMessage, packInfo.ResolvedPackageId, packInfo.Version)); var tempDirsToDelete = new List(); - var tempFilesToDelete = new List(); - bool shouldRollBackPack = false; transactionContext.Run( action: () => { - if (PackIsInstalled(packInfo) && !overwriteExistingPacks) + if (!Directory.Exists(Path.GetDirectoryName(packInfo.Path))) + { + Directory.CreateDirectory(Path.GetDirectoryName(packInfo.Path)); + } + + if (IsSingleFilePack(packInfo)) { - _reporter.WriteLine(string.Format(LocalizableStrings.WorkloadPackAlreadyInstalledMessage, packInfo.ResolvedPackageId, packInfo.Version)); + File.Copy(packagePath, packInfo.Path, overwrite: overwriteExistingPacks); } else { - shouldRollBackPack = true; - string packagePath; - if (offlineCache == null || !offlineCache.HasValue) - { - packagePath = _nugetPackageDownloader - .DownloadPackageAsync(new PackageId(packInfo.ResolvedPackageId), - new NuGetVersion(packInfo.Version), - _packageSourceLocation).GetAwaiter().GetResult(); - tempFilesToDelete.Add(packagePath); - } - else - { - _reporter.WriteLine(string.Format(LocalizableStrings.UsingCacheForPackInstall, packInfo.ResolvedPackageId, packInfo.Version, offlineCache)); - packagePath = Path.Combine(offlineCache.Value.Value, $"{packInfo.ResolvedPackageId}.{packInfo.Version}.nupkg"); - if (!File.Exists(packagePath)) - { - throw new Exception(string.Format(LocalizableStrings.CacheMissingPackage, packInfo.ResolvedPackageId, packInfo.Version, offlineCache)); - } - } + var tempExtractionDir = Path.Combine(_tempPackagesDir.Value, $"{packInfo.ResolvedPackageId}-{packInfo.Version}-extracted"); + tempDirsToDelete.Add(tempExtractionDir); - if (!Directory.Exists(Path.GetDirectoryName(packInfo.Path))) + // This directory should have been deleted, but remove it just in case + if (overwriteExistingPacks && Directory.Exists(tempExtractionDir)) { - Directory.CreateDirectory(Path.GetDirectoryName(packInfo.Path)); + Directory.Delete(tempExtractionDir, recursive: true); } - if (IsSingleFilePack(packInfo)) + Directory.CreateDirectory(tempExtractionDir); + var packFiles = _nugetPackageDownloader.ExtractPackageAsync(packagePath, new DirectoryPath(tempExtractionDir)).GetAwaiter().GetResult(); + + if (overwriteExistingPacks && Directory.Exists(packInfo.Path)) { - File.Copy(packagePath, packInfo.Path, overwrite: overwriteExistingPacks); + Directory.Delete(packInfo.Path, recursive: true); } - else - { - var tempExtractionDir = Path.Combine(_tempPackagesDir.Value, $"{packInfo.ResolvedPackageId}-{packInfo.Version}-extracted"); - tempDirsToDelete.Add(tempExtractionDir); - - // This directory should have been deleted, but remove it just in case - if (overwriteExistingPacks && Directory.Exists(tempExtractionDir)) - { - Directory.Delete(tempExtractionDir, recursive: true); - } - Directory.CreateDirectory(tempExtractionDir); - var packFiles = _nugetPackageDownloader.ExtractPackageAsync(packagePath, new DirectoryPath(tempExtractionDir)).GetAwaiter().GetResult(); - - if (overwriteExistingPacks && Directory.Exists(packInfo.Path)) - { - Directory.Delete(packInfo.Path, recursive: true); - } - - FileAccessRetrier.RetryOnMoveAccessFailure(() => DirectoryPath.MoveDirectory(tempExtractionDir, packInfo.Path)); - } - } + FileAccessRetrier.RetryOnMoveAccessFailure(() => DirectoryPath.MoveDirectory(tempExtractionDir, packInfo.Path)); + } WritePackInstallationRecord(packInfo, sdkFeatureBand); }, @@ -215,14 +223,11 @@ public void InstallWorkloads(IEnumerable workloadIds, SdkFeatureBand { try { - if (shouldRollBackPack) + _reporter.WriteLine(string.Format(LocalizableStrings.RollingBackPackInstall, packInfo.ResolvedPackageId)); + DeletePackInstallationRecord(packInfo, sdkFeatureBand); + if (!PackHasInstallRecords(packInfo)) { - _reporter.WriteLine(string.Format(LocalizableStrings.RollingBackPackInstall, packInfo.ResolvedPackageId)); - DeletePackInstallationRecord(packInfo, sdkFeatureBand); - if (!PackHasInstallRecords(packInfo)) - { - DeletePack(packInfo); - } + DeletePack(packInfo); } } catch (Exception e) From 01175f75c35c9660243bc00c22c50a4631aa4417 Mon Sep 17 00:00:00 2001 From: Forgind <12969783+Forgind@users.noreply.github.com> Date: Wed, 22 Jan 2025 13:34:16 -0800 Subject: [PATCH 2/5] Show stack trace and fix concurrency issue? --- .../dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs | 5 +++-- .../commands/dotnet-workload/install/FileBasedInstaller.cs | 4 ++-- .../commands/dotnet-workload/update/WorkloadUpdateCommand.cs | 4 ++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs b/src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs index 87de51b002fe..c6b509a44452 100644 --- a/src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs +++ b/src/Cli/dotnet/NugetPackageDownloader/NuGetPackageDownloader.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Collections.Concurrent; using Microsoft.DotNet.Cli.Utils; using Microsoft.DotNet.ToolPackage; using Microsoft.DotNet.Tools; @@ -35,7 +36,7 @@ internal class NuGetPackageDownloader : INuGetPackageDownloader private readonly IReporter _reporter; private readonly IFirstPartyNuGetPackageSigningVerifier _firstPartyNuGetPackageSigningVerifier; private bool _validationMessagesDisplayed = false; - private readonly Dictionary _sourceRepositories; + private readonly ConcurrentDictionary _sourceRepositories; private readonly bool _shouldUsePackageSourceMapping; private readonly bool _verifySignatures; @@ -818,7 +819,7 @@ private SourceRepository GetSourceRepository(PackageSource source) if (!_sourceRepositories.TryGetValue(source, out SourceRepository value)) { value = Repository.Factory.GetCoreV3(source); - _sourceRepositories.Add(source, value); + _sourceRepositories.AddOrUpdate(source, _ => value, (_, _) => value); } return value; diff --git a/src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs b/src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs index a1699f5ff710..e9a33939fb13 100644 --- a/src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs +++ b/src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs @@ -289,9 +289,9 @@ public void InstallWorkloadManifest(ManifestVersionUpdate manifestUpdate, ITrans RemoveManifestInstallationRecord(manifestUpdate.ManifestId, manifestUpdate.NewVersion, new SdkFeatureBand(manifestUpdate.NewFeatureBand), _sdkFeatureBand); }); } - catch (Exception e) + catch (Exception) { - throw new Exception(string.Format(LocalizableStrings.FailedToInstallWorkloadManifest, manifestUpdate.ManifestId, manifestUpdate.NewVersion, e.Message), e); + throw; // new Exception(string.Format(LocalizableStrings.FailedToInstallWorkloadManifest, manifestUpdate.ManifestId, manifestUpdate.NewVersion, e.Message), e); } } diff --git a/src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs b/src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs index 4ecc1e10993f..0855e86cd4fd 100644 --- a/src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs +++ b/src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs @@ -129,10 +129,10 @@ public override int Execute() UpdateWorkloads(); } } - catch (Exception e) + catch (Exception) { // Don't show entire stack trace - throw new GracefulException(string.Format(LocalizableStrings.WorkloadUpdateFailed, e.Message), e, isUserError: false); + throw; // new GracefulException(string.Format(LocalizableStrings.WorkloadUpdateFailed, e.Message), e, isUserError: false); } } From 911866d4962893c7ac196fc449af6507d534677a Mon Sep 17 00:00:00 2001 From: Forgind <12969783+Forgind@users.noreply.github.com> Date: Wed, 22 Jan 2025 17:34:52 -0800 Subject: [PATCH 3/5] Revert stack changes --- .../commands/dotnet-workload/install/FileBasedInstaller.cs | 4 ++-- .../commands/dotnet-workload/update/WorkloadUpdateCommand.cs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs b/src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs index e9a33939fb13..a1699f5ff710 100644 --- a/src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs +++ b/src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs @@ -289,9 +289,9 @@ public void InstallWorkloadManifest(ManifestVersionUpdate manifestUpdate, ITrans RemoveManifestInstallationRecord(manifestUpdate.ManifestId, manifestUpdate.NewVersion, new SdkFeatureBand(manifestUpdate.NewFeatureBand), _sdkFeatureBand); }); } - catch (Exception) + catch (Exception e) { - throw; // new Exception(string.Format(LocalizableStrings.FailedToInstallWorkloadManifest, manifestUpdate.ManifestId, manifestUpdate.NewVersion, e.Message), e); + throw new Exception(string.Format(LocalizableStrings.FailedToInstallWorkloadManifest, manifestUpdate.ManifestId, manifestUpdate.NewVersion, e.Message), e); } } diff --git a/src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs b/src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs index 0855e86cd4fd..3cd8ae6920cb 100644 --- a/src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs +++ b/src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs @@ -132,7 +132,7 @@ public override int Execute() catch (Exception) { // Don't show entire stack trace - throw; // new GracefulException(string.Format(LocalizableStrings.WorkloadUpdateFailed, e.Message), e, isUserError: false); + throw new GracefulException(string.Format(LocalizableStrings.WorkloadUpdateFailed, e.Message), e, isUserError: false); } } From 0ad86bdaad855115bfeb343561653dc7a0956fa0 Mon Sep 17 00:00:00 2001 From: Forgind <12969783+Forgind@users.noreply.github.com> Date: Wed, 22 Jan 2025 17:35:53 -0800 Subject: [PATCH 4/5] One more --- .../commands/dotnet-workload/update/WorkloadUpdateCommand.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs b/src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs index 3cd8ae6920cb..4ecc1e10993f 100644 --- a/src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs +++ b/src/Cli/dotnet/commands/dotnet-workload/update/WorkloadUpdateCommand.cs @@ -129,7 +129,7 @@ public override int Execute() UpdateWorkloads(); } } - catch (Exception) + catch (Exception e) { // Don't show entire stack trace throw new GracefulException(string.Format(LocalizableStrings.WorkloadUpdateFailed, e.Message), e, isUserError: false); From 55dd0f2eb609bcbebb90af3dbaec1e24d8d4b0e5 Mon Sep 17 00:00:00 2001 From: Forgind <12969783+Forgind@users.noreply.github.com> Date: Wed, 5 Feb 2025 17:32:34 -0800 Subject: [PATCH 5/5] Little fix --- .../dotnet-workload/install/FileBasedInstaller.cs | 1 + .../GivenFileBasedWorkloadInstall.cs | 8 ++++---- .../MockNuGetPackageInstaller.cs | 9 ++++++++- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs b/src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs index a1699f5ff710..f7836e920c0b 100644 --- a/src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs +++ b/src/Cli/dotnet/commands/dotnet-workload/install/FileBasedInstaller.cs @@ -147,6 +147,7 @@ public void InstallWorkloads(IEnumerable workloadIds, SdkFeatureBand if (PackIsInstalled(packInfo) && !overwriteExistingPacks) { _reporter.WriteLine(string.Format(LocalizableStrings.WorkloadPackAlreadyInstalledMessage, packInfo.ResolvedPackageId, packInfo.Version)); + WritePackInstallationRecord(packInfo, sdkFeatureBand); } else { diff --git a/test/dotnet-workload-install.Tests/GivenFileBasedWorkloadInstall.cs b/test/dotnet-workload-install.Tests/GivenFileBasedWorkloadInstall.cs index 3e6ce0f632ef..afac5547afeb 100644 --- a/test/dotnet-workload-install.Tests/GivenFileBasedWorkloadInstall.cs +++ b/test/dotnet-workload-install.Tests/GivenFileBasedWorkloadInstall.cs @@ -410,11 +410,11 @@ public void GivenManagedInstallItCanErrorsWhenMissingOfflineCache() var version = "6.0.100"; var cachePath = Path.Combine(dotnetRoot, "MockCache"); - var exceptionThrown = Assert.Throws(() => + var exceptionThrown = Assert.Throws(() => CliTransaction.RunNew(context => installer.InstallWorkloads(new[] { new WorkloadId("android-sdk-workload") }, new SdkFeatureBand(version), context, new DirectoryPath(cachePath)))); - exceptionThrown.Message.Should().Contain(packId); - exceptionThrown.Message.Should().Contain(packVersion); - exceptionThrown.Message.Should().Contain(cachePath); + exceptionThrown.InnerException.Message.Should().Contain(packId); + exceptionThrown.InnerException.Message.Should().Contain(packVersion); + exceptionThrown.InnerException.Message.Should().Contain(cachePath); } private (string, FileBasedInstaller, INuGetPackageDownloader, Func) GetTestInstaller([CallerMemberName] string testName = "", bool failingInstaller = false, string identifier = "", bool manifestDownload = false, diff --git a/test/dotnet-workload-install.Tests/MockNuGetPackageInstaller.cs b/test/dotnet-workload-install.Tests/MockNuGetPackageInstaller.cs index e26e9d963bbe..9fa2cb8c41e0 100644 --- a/test/dotnet-workload-install.Tests/MockNuGetPackageInstaller.cs +++ b/test/dotnet-workload-install.Tests/MockNuGetPackageInstaller.cs @@ -54,7 +54,14 @@ public Task DownloadPackageAsync(PackageId packageId, DownloadCallResult.Add(path); if (_downloadPath != string.Empty) { - File.WriteAllText(path, string.Empty); + try + { + File.WriteAllText(path, string.Empty); + } + catch (IOException) + { + // Do not write this file twice in parallel + } } _lastPackageVersion = packageVersion ?? new NuGetVersion("1.0.42"); return Task.FromResult(path);