Improve dotnetup Muxer Handling, In-Use Muxer + Perf#52696
Improve dotnetup Muxer Handling, In-Use Muxer + Perf#52696nagilson merged 13 commits intodotnet:release/dnupfrom
dotnetup Muxer Handling, In-Use Muxer + Perf#52696Conversation
When moving the .NET SDK build to use `dotnetup`, I hit several issues. The main issue is that the dotnet muxer may be in use when dotnetup tries to replace it, which would cause a failure. I don't think we want to fail in this case and instead we want to emit a warning. Maybe there should be an option to fail instead, however. What I also realized is that the approach of renaming the muxer even if it doesn't need to be replaced would cause the warning message to appear even if we didn't need to replace the muxer. Then I realized, we can do something clever. The windowsdesktop runtime has no runtime so we can skip muxer logic there. The other runtimes versions are the deciding factor for the muxer version since the muxer doesn't have its own version, so we can skip the resolution to find the version there. For the SDK, what we can do to avoid an extra rename of the existing muxer if it doesn't need to be replaced, is to extract out the new muxer to a different name temporarily, and then look and see if the latest runtime in the dotnet install root target changed after that install. If so then the runtime that came with the SDK is newer so the new muxer should be used. This lets us gracefully handle when the muxer is in use as well as avoid any extra work we don't need to do.
…etup-in-use-muxer
There was a problem hiding this comment.
Pull request overview
This PR updates dotnetup’s install flow and the underlying installation library to handle muxer (dotnet/dotnet.exe) replacement more safely and with less unnecessary work—especially when the existing muxer is in use—while adding a switch to optionally fail instead of warning.
Changes:
- Add
--require-muxer-updateand plumb it throughdotnetupworkflows into the installation library. - Refactor archive extraction to extract the muxer to a temp path first, then decide post-extraction whether to replace the installed muxer.
- Add targeted tests covering muxer replacement decisions and “muxer in use” behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/dotnetup.Tests/MuxerHandlerTests.cs | New unit tests for muxer replacement + locked-muxer scenarios |
| src/Installer/dotnetup/CommonOptions.cs | Adds --require-muxer-update option |
| src/Installer/dotnetup/Commands/Shared/InstallWorkflow.cs | Threads RequireMuxerUpdate through workflow options |
| src/Installer/dotnetup/Commands/Shared/InstallExecutor.cs | Propagates RequireMuxerUpdate into install request options |
| src/Installer/dotnetup/Commands/Sdk/Install/SdkInstallCommandParser.cs | Exposes + registers the new option for SDK installs |
| src/Installer/dotnetup/Commands/Sdk/Install/SdkInstallCommand.cs | Passes RequireMuxerUpdate into the workflow |
| src/Installer/dotnetup/Commands/Runtime/Install/RuntimeInstallCommandParser.cs | Registers the new option for runtime installs |
| src/Installer/dotnetup/Commands/Runtime/Install/RuntimeInstallCommand.cs | Intended to pass RequireMuxerUpdate, but currently contains unresolved conflict |
| src/Installer/Microsoft.Dotnet.Installation/Microsoft.Dotnet.Installation.csproj | Adds Spectre.Console dependency to installation library |
| src/Installer/Microsoft.Dotnet.Installation/Internal/MuxerHandler.cs | New muxer handling logic (temp extract + conditional replace + in-use handling) |
| src/Installer/Microsoft.Dotnet.Installation/Internal/DotnetInstall.cs | Adds RequireMuxerUpdate to install request options |
| src/Installer/Microsoft.Dotnet.Installation/Internal/DotnetArchiveExtractor.cs | Integrates MuxerHandler into tar/zip extraction |
src/Installer/dotnetup/Commands/Runtime/Install/RuntimeInstallCommand.cs
Outdated
Show resolved
Hide resolved
src/Installer/Microsoft.Dotnet.Installation/Internal/MuxerHandler.cs
Outdated
Show resolved
Hide resolved
src/Installer/Microsoft.Dotnet.Installation/Internal/MuxerHandler.cs
Outdated
Show resolved
Hide resolved
src/Installer/Microsoft.Dotnet.Installation/Microsoft.Dotnet.Installation.csproj
Outdated
Show resolved
Hide resolved
could go in a separate PR and is somewhat opinionated - but I think the intent is clear to install to a specific path.
src/Installer/Microsoft.Dotnet.Installation/Internal/MuxerHandler.cs
Outdated
Show resolved
Hide resolved
dsplaisted
left a comment
There was a problem hiding this comment.
Looks good.
The windowsdesktop runtime has no runtime so we can skip muxer logic there. The other runtimes versions are the deciding factor for the muxer version since the muxer doesn't have its own version, so we can skip the resolution to find the version there.
Is there any logic specific to whether we're installing an SDK or which shared framework we're installing? It seems like there's not (which seems like a good thing, it seems better just to base it off of the files we're extracting.
It's unfortunate that having a running dotnet app prevents the muxer from being overwritten. I kind of thought that it shouldn't work that way.
If we are going to fail it seems like it would be good to fail before we start extraction. We should be able to look at the archive and determine whether the muxer is newer than the existing version. So could our first operation be to move the existing muxer if we need to overwrite it? Then if we fail we won't have extracted any other files. That seems like a better experience, especially if you think we should change the default to fail if the muxer can't be overwritten.
src/Installer/Microsoft.Dotnet.Installation/Internal/MuxerHandler.cs
Outdated
Show resolved
Hide resolved
src/Installer/Microsoft.Dotnet.Installation/Internal/MuxerHandler.cs
Outdated
Show resolved
Hide resolved
src/Installer/Microsoft.Dotnet.Installation/Internal/MuxerHandler.cs
Outdated
Show resolved
Hide resolved
src/Installer/Microsoft.Dotnet.Installation/Internal/MuxerHandler.cs
Outdated
Show resolved
Hide resolved
src/Installer/Microsoft.Dotnet.Installation/Internal/DotnetArchiveExtractor.cs
Show resolved
Hide resolved
src/Installer/Microsoft.Dotnet.Installation/Internal/MuxerHandler.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Daniel Plaisted <dsplaisted@gmail.com>
I concur, there was at some point but I removed it so we would be flexible if the .zip format changed. I like the idea of moving the muxer but I think this is risky with the lock implementations and race conditions. Instead we can simply ping to see if it's in use during |
@nagilson I'm not sure I get how it would be any more risky than what this PR does. It would be inside the lock, and I think the move is the same or less likely to fail than deleting it. It seems better to fail up-front rather than failing after we've already extracted everything. |
|
Happy to change this, but my thinking is 2-fold:
I think the best approach for now is to check the muxer usage with the initial lock that sees if the install already exists, and then we will also fail elegantly if that changes between then and the |
Fixes #51691
This also addresses the exception raised in this issue #51689
When moving the .NET SDK build to use
dotnetupin nagilson#8, I hit several issues. The main issue is that the dotnet muxer may be in use when dotnetup tries to replace it, which would cause a failure.I don't think we want to fail in this case and instead we want to emit a warning. I've added an option to fail instead, however.
What I also realized is that the approach of renaming the muxer even if it doesn't need to be replaced would cause the warning message to appear even if we didn't need to replace the muxer. Then I realized, we can do something clever. The windowsdesktop runtime has no runtime so we can skip muxer logic there. The other runtimes versions are the deciding factor for the muxer version since the muxer doesn't have its own version, so we can skip the resolution to find the version there.
For the SDK, what we can do to avoid an extra rename of the existing muxer if it doesn't need to be replaced, is to extract out the new muxer to a different name temporarily, and then look and see if the latest runtime in the dotnet install root target changed after that install. If so then the runtime that came with the SDK is newer so the new muxer should be used. This lets us gracefully handle when the muxer is in use as well as avoid any extra work we don't need to do.
#52696 this will fail quite often until this other PR is merged due to a dotnetup bug.