Code review feedback and other fixes

dsplaisted · dsplaisted · commit bc8ec5870415 · 2025-11-11T13:42:24.000-05:00
diff --git a/src/Installer/Microsoft.Dotnet.Installation/Internal/ChannelVersionResolver.cs b/src/Installer/Microsoft.Dotnet.Installation/Internal/ChannelVersionResolver.cs
@@ -88,7 +88,7 @@ static IEnumerable<string> GetChannelsForProduct(Product product)
     /// Finds the latest fully specified version for a given channel string (major, major.minor, or feature band).
     /// </summary>
     /// <param name="channel">Channel string (e.g., "9", "9.0", "9.0.1xx", "9.0.103", "lts", "sts", "preview")</param>
-    /// <param name="mode">InstallMode.SDK or InstallMode.Runtime</param>
+    /// <param name="component">The component to check (ie SDK or runtime)</param>
     /// <returns>Latest fully specified version string, or null if not found</returns>
     public ReleaseVersion? GetLatestVersionForChannel(UpdateChannel channel, InstallComponent component)
     {
@@ -143,7 +143,7 @@ static IEnumerable<string> GetChannelsForProduct(Product product)
 
     private IEnumerable<Product> GetProductsInMajorOrMajorMinor(IEnumerable<Product> index, int major, int? minor = null)
     {
-        var validProducts = index.Where(p => p.ProductVersion.StartsWith(minor is not null ? $"{major}.{minor}" : $"{major}."));
+        var validProducts = index.Where(p => minor is not null ? p.ProductVersion.Equals($"{major}.{minor}") : p.ProductVersion.StartsWith($"{major}."));
         return validProducts;
     }
 
@@ -161,8 +161,8 @@ private IEnumerable<Product> GetProductsInMajorOrMajorMinor(IEnumerable<Product>
     /// Gets the latest version based on support status (LTS or STS).
     /// </summary>
     /// <param name="index">The product collection to search</param>
-    /// <param name="isLts">True for LTS (Long-Term Support), false for STS (Standard-Term Support)</param>
-    /// <param name="mode">InstallComponent.SDK or InstallComponent.Runtime</param>
+    /// <param name="releaseType">The release type to filter by (LTS or STS)</param>
+    /// <param name="component">The component to check (ie SDK or runtime)</param>
     /// <returns>Latest stable version string matching the support status, or null if none found</returns>
     private static ReleaseVersion? GetLatestVersionByReleaseType(IEnumerable<Product> index, ReleaseType releaseType, InstallComponent component)
     {
@@ -174,7 +174,7 @@ private IEnumerable<Product> GetProductsInMajorOrMajorMinor(IEnumerable<Product>
     /// Gets the latest preview version available.
     /// </summary>
     /// <param name="index">The product collection to search</param>
-    /// <param name="mode">InstallComponent.SDK or InstallComponent.Runtime</param>
+    /// <param name="component">The component to check (ie SDK or runtime)</param>
     /// <returns>Latest preview or GoLive version string, or null if none found</returns>
     private ReleaseVersion? GetLatestPreviewVersion(IEnumerable<Product> index, InstallComponent component)
     {
diff --git a/src/Installer/Microsoft.Dotnet.Installation/Internal/DotnetArchiveDownloader.cs b/src/Installer/Microsoft.Dotnet.Installation/Internal/DotnetArchiveDownloader.cs
@@ -19,23 +19,33 @@ namespace Microsoft.Dotnet.Installation.Internal;
 /// <summary>
 /// Handles downloading and parsing .NET release manifests to find the correct installer/archive for a given installation.
 /// </summary>
-internal class DotnetArchiveDownloader(HttpClient httpClient) : IDisposable
+internal class DotnetArchiveDownloader : IDisposable
 {
     private const int MaxRetryCount = 3;
     private const int RetryDelayMilliseconds = 1000;
 
-    private readonly HttpClient _httpClient = httpClient ?? throw new ArgumentNullException(nameof(httpClient));
-    private ReleaseManifest _releaseManifest = new();
+    private readonly HttpClient _httpClient;
+    private readonly bool _shouldDisposeHttpClient;
+    private ReleaseManifest _releaseManifest;
 
     public DotnetArchiveDownloader()
-        : this(CreateDefaultHttpClient())
+        : this(new ReleaseManifest())
     {
     }
 
-    public DotnetArchiveDownloader(ReleaseManifest releaseManifest)
-        : this(CreateDefaultHttpClient())
+    public DotnetArchiveDownloader(ReleaseManifest releaseManifest, HttpClient? httpClient = null)
     {
         _releaseManifest = releaseManifest ?? throw new ArgumentNullException(nameof(releaseManifest));
+        if (httpClient == null)
+        {
+            _httpClient = CreateDefaultHttpClient();
+            _shouldDisposeHttpClient = true;
+        }
+        else
+        {
+            _httpClient = httpClient;
+            _shouldDisposeHttpClient = false;
+        }
     }
 
     /// <summary>
@@ -69,8 +79,7 @@ private static HttpClient CreateDefaultHttpClient()
     /// <param name="downloadUrl">The URL to download from</param>
     /// <param name="destinationPath">The local path to save the downloaded file</param>
     /// <param name="progress">Optional progress reporting</param>
-    /// <returns>True if download was successful, false otherwise</returns>
-    protected async Task<bool> DownloadArchiveAsync(string downloadUrl, string destinationPath, IProgress<DownloadProgress>? progress = null)
+    async Task DownloadArchiveAsync(string downloadUrl, string destinationPath, IProgress<DownloadProgress>? progress = null)
     {
         // Create temp file path in same directory for atomic move when complete
         string tempPath = $"{destinationPath}.download";
@@ -82,15 +91,14 @@ protected async Task<bool> DownloadArchiveAsync(string downloadUrl, string desti
                 // Ensure the directory exists
                 Directory.CreateDirectory(Path.GetDirectoryName(destinationPath)!);
 
-                // Try to get content length for progress reporting
-                long? totalBytes = await GetContentLengthAsync(downloadUrl);
+                // Content length for progress reporting
+                long? totalBytes = null;
 
                 // Make the actual download request
                 using var response = await _httpClient.GetAsync(downloadUrl, HttpCompletionOption.ResponseHeadersRead);
                 response.EnsureSuccessStatusCode();
 
-                // Get the total bytes if we didn't get it before
-                if (!totalBytes.HasValue && response.Content.Headers.ContentLength.HasValue)
+                if (response.Content.Headers.ContentLength.HasValue)
                 {
                     totalBytes = response.Content.Headers.ContentLength.Value;
                 }
@@ -102,7 +110,7 @@ protected async Task<bool> DownloadArchiveAsync(string downloadUrl, string desti
                 long bytesRead = 0;
                 int read;
 
-                var lastProgressReport = DateTime.UtcNow;
+                var lastProgressReport = DateTime.MinValue;
 
                 while ((read = await contentStream.ReadAsync(buffer)) > 0)
                 {
@@ -133,9 +141,20 @@ protected async Task<bool> DownloadArchiveAsync(string downloadUrl, string desti
                 }
                 File.Move(tempPath, destinationPath);
 
-                return true;
+                return;
             }
             catch (Exception)
+            {
+                if (attempt < MaxRetryCount)
+                {
+                    await Task.Delay(RetryDelayMilliseconds * attempt); // Linear backoff
+                }
+                else
+                {
+                    throw;
+                }
+            }
+            finally
             {
                 // Delete the partial download if it exists
                 try
@@ -150,35 +169,9 @@ protected async Task<bool> DownloadArchiveAsync(string downloadUrl, string desti
                     // Ignore cleanup errors
                 }
 
-                if (attempt < MaxRetryCount)
-                {
-                    await Task.Delay(RetryDelayMilliseconds * attempt); // Exponential backoff
-                }
-                else
-                {
-                    return false;
-                }
             }
         }
 
-        return false;
-    }
-
-    /// <summary>
-    /// Gets the content length of a resource.
-    /// </summary>
-    private async Task<long?> GetContentLengthAsync(string url)
-    {
-        try
-        {
-            using var headRequest = new HttpRequestMessage(HttpMethod.Head, url);
-            using var headResponse = await _httpClient.SendAsync(headRequest);
-            return headResponse.Content.Headers.ContentLength;
-        }
-        catch
-        {
-            return null;
-        }
     }
 
     /// <summary>
@@ -187,10 +180,9 @@ protected async Task<bool> DownloadArchiveAsync(string downloadUrl, string desti
     /// <param name="downloadUrl">The URL to download from</param>
     /// <param name="destinationPath">The local path to save the downloaded file</param>
     /// <param name="progress">Optional progress reporting</param>
-    /// <returns>True if download was successful, false otherwise</returns>
-    protected bool DownloadArchive(string downloadUrl, string destinationPath, IProgress<DownloadProgress>? progress = null)
+    void DownloadArchive(string downloadUrl, string destinationPath, IProgress<DownloadProgress>? progress = null)
     {
-        return DownloadArchiveAsync(downloadUrl, destinationPath, progress).GetAwaiter().GetResult();
+        DownloadArchiveAsync(downloadUrl, destinationPath, progress).GetAwaiter().GetResult();
     }
 
     /// <summary>
@@ -200,23 +192,24 @@ protected bool DownloadArchive(string downloadUrl, string destinationPath, IProg
     /// <param name="destinationPath">The local path to save the downloaded file</param>
     /// <param name="progress">Optional progress reporting</param>
     /// <returns>True if download and verification were successful, false otherwise</returns>
-    public bool DownloadArchiveWithVerification(DotnetInstallRequest installRequest, ReleaseVersion resolvedVersion, string destinationPath, IProgress<DownloadProgress>? progress = null)
+    public void DownloadArchiveWithVerification(DotnetInstallRequest installRequest, ReleaseVersion resolvedVersion, string destinationPath, IProgress<DownloadProgress>? progress = null)
     {
         var targetFile = _releaseManifest.FindReleaseFile(installRequest, resolvedVersion);
         string? downloadUrl = targetFile?.Address.ToString();
         string? expectedHash = targetFile?.Hash.ToString();
 
-        if (string.IsNullOrEmpty(expectedHash) || string.IsNullOrEmpty(downloadUrl))
+        if (string.IsNullOrEmpty(expectedHash))
         {
-            return false;
+            throw new ArgumentException($"{nameof(expectedHash)} cannot be null or empty");
         }
-
-        if (!DownloadArchive(downloadUrl, destinationPath, progress))
+        if (string.IsNullOrEmpty(downloadUrl))
         {
-            return false;
+            throw new ArgumentException($"{nameof(downloadUrl)} cannot be null or empty");
         }
 
-        return VerifyFileHash(destinationPath, expectedHash);
+        DownloadArchive(downloadUrl, destinationPath, progress);
+
+        VerifyFileHash(destinationPath, expectedHash);
     }
 
 
@@ -240,20 +233,25 @@ public static string ComputeFileHash(string filePath)
     /// </summary>
     /// <param name="filePath">Path to the file to verify</param>
     /// <param name="expectedHash">Expected hash value</param>
-    /// <returns>True if the hash matches, false otherwise</returns>
-    public static bool VerifyFileHash(string filePath, string expectedHash)
+    public static void VerifyFileHash(string filePath, string expectedHash)
     {
         if (string.IsNullOrEmpty(expectedHash))
         {
-            return false;
+            throw new ArgumentException("Expected hash cannot be null or empty", nameof(expectedHash));
         }
 
         string actualHash = ComputeFileHash(filePath);
-        return string.Equals(actualHash, expectedHash, StringComparison.OrdinalIgnoreCase);
+        if (!string.Equals(actualHash, expectedHash, StringComparison.OrdinalIgnoreCase))
+        {
+            throw new Exception($"File hash mismatch. Expected: {expectedHash}, Actual: {actualHash}");
+        }
     }
 
     public void Dispose()
     {
-        _httpClient?.Dispose();
+        if (_shouldDisposeHttpClient)
+        {
+            _httpClient?.Dispose();
+        }
     }
 }
diff --git a/src/Installer/Microsoft.Dotnet.Installation/Internal/DotnetArchiveExtractor.cs b/src/Installer/Microsoft.Dotnet.Installation/Internal/DotnetArchiveExtractor.cs
@@ -19,7 +19,6 @@ internal class DotnetArchiveExtractor : IDisposable
     private readonly DotnetInstallRequest _request;
     private readonly ReleaseVersion _resolvedVersion;
     private readonly IProgressTarget _progressTarget;
-    private readonly ReleaseManifest _releaseManifest;
     private string scratchDownloadDirectory;
     private string? _archivePath;
 
@@ -28,26 +27,29 @@ public DotnetArchiveExtractor(DotnetInstallRequest request, ReleaseVersion resol
         _request = request;
         _resolvedVersion = resolvedVersion;
         _progressTarget = progressTarget;
-        _releaseManifest = new();
         scratchDownloadDirectory = Directory.CreateTempSubdirectory().FullName;
     }
 
     public void Prepare()
     {
         using var activity = InstallationActivitySource.ActivitySource.StartActivity("DotnetInstaller.Prepare");
 
-        using var archiveDownloader = new DotnetArchiveDownloader(_releaseManifest);
+        using var archiveDownloader = new DotnetArchiveDownloader();
         var archiveName = $"dotnet-{Guid.NewGuid()}";
         _archivePath = Path.Combine(scratchDownloadDirectory, archiveName + DnupUtilities.GetArchiveFileExtensionForPlatform());
 
         using (var progressReporter = _progressTarget.CreateProgressReporter())
         {
             var downloadTask = progressReporter.AddTask($"Downloading .NET SDK {_resolvedVersion}", 100);
             var reporter = new DownloadProgressReporter(downloadTask, $"Downloading .NET SDK {_resolvedVersion}");
-            var downloadSuccess = archiveDownloader.DownloadArchiveWithVerification(_request, _resolvedVersion, _archivePath, reporter);
-            if (!downloadSuccess)
+
+            try
+            {
+                archiveDownloader.DownloadArchiveWithVerification(_request, _resolvedVersion, _archivePath, reporter);
+            }
+            catch (Exception ex)
             {
-                throw new InvalidOperationException($"Failed to download .NET archive for version {_resolvedVersion}");
+                throw new Exception($"Failed to download .NET archive for version {_resolvedVersion}", ex);
             }
 
             downloadTask.Value = 100;
@@ -242,7 +244,7 @@ private void ExtractTarFileEntry(TarEntry entry, string targetDir, MuxerHandling
     private void HandleMuxerUpdateFromTar(TarEntry entry, string muxerTargetPath)
     {
         // Create a temporary file for the muxer first to avoid locking issues
-        var tempMuxerPath = Directory.CreateTempSubdirectory().FullName;
+        var tempMuxerPath = Path.Combine(Directory.CreateTempSubdirectory().FullName, entry.Name);
         using (var outStream = File.Create(tempMuxerPath))
         {
             entry.DataStream?.CopyTo(outStream);
@@ -327,7 +329,7 @@ private void ExtractZipEntry(ZipArchiveEntry entry, string targetDir, MuxerHandl
      */
     private void HandleMuxerUpdateFromZip(ZipArchiveEntry entry, string muxerTargetPath)
     {
-        var tempMuxerPath = Directory.CreateTempSubdirectory().FullName;
+        var tempMuxerPath = Path.Combine(Directory.CreateTempSubdirectory().FullName, entry.Name);
         entry.ExtractToFile(tempMuxerPath, overwrite: true);
 
         try
diff --git a/src/Installer/Microsoft.Dotnet.Installation/Internal/ScopedMutex.cs b/src/Installer/Microsoft.Dotnet.Installation/Internal/ScopedMutex.cs
@@ -15,26 +15,18 @@ public class ScopedMutex : IDisposable
 
     public ScopedMutex(string name)
     {
-        try
+        // On Linux and Mac, "Global\" prefix doesn't work - strip it if present
+        string mutexName = name;
+        if (Environment.OSVersion.Platform != PlatformID.Win32NT && mutexName.StartsWith("Global\\"))
         {
-            // On Linux and Mac, "Global\" prefix doesn't work - strip it if present
-            string mutexName = name;
-            if (Environment.OSVersion.Platform != PlatformID.Win32NT && mutexName.StartsWith("Global\\"))
-            {
-                mutexName = mutexName.Substring(7);
-            }
-
-            _mutex = new Mutex(false, mutexName);
-            _hasHandle = _mutex.WaitOne(TimeSpan.FromSeconds(300), false);
-            if (_hasHandle)
-            {
-                _holdCount.Value = _holdCount.Value + 1;
-            }
+            mutexName = mutexName.Substring(7);
         }
-        catch (Exception ex)
+
+        _mutex = new Mutex(false, mutexName);
+        _hasHandle = _mutex.WaitOne(TimeSpan.FromSeconds(300), false);
+        if (_hasHandle)
         {
-            Console.WriteLine($"Warning: Could not create or acquire mutex '{name}': {ex.Message}");
-            throw;
+            _holdCount.Value = _holdCount.Value + 1;
         }
     }
 
diff --git a/src/Installer/dnup/DotnetInstallManager.cs b/src/Installer/dnup/DotnetInstallManager.cs
@@ -36,7 +36,7 @@ public DotnetInstallManager(IEnvironmentProvider? environmentProvider = null)
         string programFiles = Environment.GetFolderPath(Environment.SpecialFolder.ProgramFiles);
         string programFilesX86 = Environment.GetFolderPath(Environment.SpecialFolder.ProgramFilesX86);
         bool isAdminInstall = installDir.StartsWith(Path.Combine(programFiles, "dotnet"), StringComparison.OrdinalIgnoreCase) ||
-                              installDir.StartsWith(Path.Combine(programFilesX86, "dotnet"), StringComparison.OrdinalIgnoreCase); // TODO: This should be improved to not be windows-specific
+                              installDir.StartsWith(Path.Combine(programFilesX86, "dotnet"), StringComparison.OrdinalIgnoreCase); // TODO: This should be improved to not be windows-specific https://github.com/dotnet/sdk/issues/51601
 
         var installRoot = new DotnetInstallRoot(installDir, InstallerUtilities.GetDefaultInstallArchitecture());
 
diff --git a/test/dnup.Tests/ChannelVersionResolverTests.cs b/test/dnup.Tests/ChannelVersionResolverTests.cs
diff --git a/test/dnup.Tests/DnupE2Etest.cs b/test/dnup.Tests/DnupE2Etest.cs

Original file line number	Diff line number	Diff line change
`@@ -15,26 +15,18 @@ public class ScopedMutex : IDisposable`
`15`	`15`
`16`	`16`	`public ScopedMutex(string name)`
`17`	`17`	`{`
`18`		`- try`
	`18`	`+ // On Linux and Mac, "Global\" prefix doesn't work - strip it if present`
	`19`	`+ string mutexName = name;`
	`20`	`+ if (Environment.OSVersion.Platform != PlatformID.Win32NT && mutexName.StartsWith("Global\\"))`
`19`	`21`	`{`
`20`		`- // On Linux and Mac, "Global\" prefix doesn't work - strip it if present`
`21`		`- string mutexName = name;`
`22`		`- if (Environment.OSVersion.Platform != PlatformID.Win32NT && mutexName.StartsWith("Global\\"))`
`23`		`- {`
`24`		`- mutexName = mutexName.Substring(7);`
`25`		`- }`
`26`		`-`
`27`		`- _mutex = new Mutex(false, mutexName);`
`28`		`- _hasHandle = _mutex.WaitOne(TimeSpan.FromSeconds(300), false);`
`29`		`- if (_hasHandle)`
`30`		`- {`
`31`		`- _holdCount.Value = _holdCount.Value + 1;`
`32`		`- }`
	`22`	`+ mutexName = mutexName.Substring(7);`
`33`	`23`	`}`
`34`		`- catch (Exception ex)`
	`24`	`+`
	`25`	`+ _mutex = new Mutex(false, mutexName);`
	`26`	`+ _hasHandle = _mutex.WaitOne(TimeSpan.FromSeconds(300), false);`
	`27`	`+ if (_hasHandle)`
`35`	`28`	`{`
`36`		`- Console.WriteLine($"Warning: Could not create or acquire mutex '{name}': {ex.Message}");`
`37`		`- throw;`
	`29`	`+ _holdCount.Value = _holdCount.Value + 1;`
`38`	`30`	`}`
`39`	`31`	`}`
`40`	`32`