commit 741244e0363578bccc7d5e0052ee108d00c7d153 parent dddc0941b89a31aaef360331abf98eb4250c9005 Author: JohnMcPMS <johnmcp@microsoft.com> Date: Thu, 30 Mar 2023 17:16:19 -0700 Enforce single install across winget processes (#3118) This change uses a named mutex to enforce a single install/uninstall operation is occurring for the entire session. The current behavior is that every process was able to install a single thing at a time, but as we have more usage this could lead to a potential issue. The mutex is acquired and held for the workflow task that routes out to the installer type specific tasks. Some installer types are explicitly excluded, as they are known to contain their own synchronization internally. In addition to the mutex, the ability to set a message to show with the indefinite progress (spinner) was added. This allows for the message to be removed when the wait is over. The message was not implemented into the progress bar, nor is it being sent to COM progress. The COM scheduling was not made aware of the mutex, and so it will execute as it has in the past, blocking on the mutex when appropriate. This could be improved in the future to enable the parallel capable installer types to operate. One potential option would be a second installer processing thread that only operated on the excluded types, and thus would never block. Diffstat:
24 files changed, 753 insertions(+), 435 deletions(-)
diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt @@ -75,6 +75,7 @@ corecrt count'th countof countryregion +CPIL cplusplus createmanifestmetadata cswinrt diff --git a/src/AppInstallerCLICore/COMContext.cpp b/src/AppInstallerCLICore/COMContext.cpp @@ -41,6 +41,11 @@ namespace AppInstaller::CLI::Execution FireCallbacks(ReportType::Progressing, current, maximum, progressType, m_executionStage); } + void COMContext::SetProgressMessage(std::string_view) + { + // TODO: Consider sending message to COM progress + } + void COMContext::EndProgress(bool) { FireCallbacks(ReportType::EndProgress, 0, 0, ProgressType::None, m_executionStage); diff --git a/src/AppInstallerCLICore/COMContext.h b/src/AppInstallerCLICore/COMContext.h @@ -54,6 +54,7 @@ namespace AppInstaller::CLI::Execution // IProgressSink void BeginProgress() override; void OnProgress(uint64_t current, uint64_t maximum, ProgressType type) override; + void SetProgressMessage(std::string_view message) override; void EndProgress(bool) override; //Execution::Context diff --git a/src/AppInstallerCLICore/ChannelStreams.cpp b/src/AppInstallerCLICore/ChannelStreams.cpp @@ -9,6 +9,19 @@ namespace AppInstaller::CLI::Execution using namespace Settings; using namespace VirtualTerminal; + size_t GetConsoleWidth() + { + CONSOLE_SCREEN_BUFFER_INFO consoleInfo{}; + if (GetConsoleScreenBufferInfo(GetStdHandle(STD_OUTPUT_HANDLE), &consoleInfo)) + { + return static_cast<size_t>(consoleInfo.dwSize.X); + } + else + { + return 120; + } + } + BaseStream::BaseStream(std::ostream& out, bool enabled, bool VTEnabled) : m_out(out), m_enabled(enabled), m_VTEnabled(VTEnabled) {} diff --git a/src/AppInstallerCLICore/ChannelStreams.h b/src/AppInstallerCLICore/ChannelStreams.h @@ -11,6 +11,9 @@ namespace AppInstaller::CLI::Execution { + // Gets the current console width. + size_t GetConsoleWidth(); + // The base stream for all channels. struct BaseStream { diff --git a/src/AppInstallerCLICore/ExecutionProgress.cpp b/src/AppInstallerCLICore/ExecutionProgress.cpp @@ -151,6 +151,23 @@ namespace AppInstaller::CLI::Execution LOG_HR(E_UNEXPECTED); } } + + void ProgressVisualizerBase::ClearLine() + { + if (UseVT()) + { + m_out << TextModification::EraseLineEntirely << '\r'; + } + else + { + m_out << '\r' << std::string(GetConsoleWidth(), ' ') << '\r'; + } + } + + void ProgressVisualizerBase::SetMessage(std::string_view message) + { + m_message = message; + } } void IndefiniteSpinner::ShowSpinner() @@ -188,17 +205,19 @@ namespace AppInstaller::CLI::Execution } // Indent two spaces for the spinner, but three here so that we can overwrite it in the loop. - m_out << " "; + std::string_view indent = " "; for (size_t i = 0; !m_canceled; ++i) { constexpr size_t repetitionCount = 20; ApplyStyle(i % repetitionCount, repetitionCount, true); - m_out << '\b' << spinnerChars[i % ARRAYSIZE(spinnerChars)] << std::flush; + m_out << '\r' << indent << spinnerChars[i % ARRAYSIZE(spinnerChars)]; + m_out.RestoreDefault(); + m_out << ' ' << m_message << std::flush; Sleep(250); } - m_out << "\b \r"; + ClearLine(); if (UseVT()) { @@ -217,6 +236,7 @@ namespace AppInstaller::CLI::Execution ClearLine(); } + // TODO: Progress bar does not currently use message if (UseVT()) { ShowProgressWithVT(current, maximum, type); @@ -255,19 +275,6 @@ namespace AppInstaller::CLI::Execution } } - void ProgressBar::ClearLine() - { - if (UseVT()) - { - m_out << TextModification::EraseLineEntirely << '\r'; - } - else - { - // Best effort when no VT (arbitrary number of spaces that seems to work) - m_out << "\r \r"; - } - } - void ProgressBar::ShowProgressNoVT(uint64_t current, uint64_t maximum, ProgressType type) { m_out << "\r "; diff --git a/src/AppInstallerCLICore/ExecutionProgress.h b/src/AppInstallerCLICore/ExecutionProgress.h @@ -27,15 +27,20 @@ namespace AppInstaller::CLI::Execution void SetStyle(AppInstaller::Settings::VisualStyle style) { m_style = style; } + void SetMessage(std::string_view message); + protected: BaseStream& m_out; Settings::VisualStyle m_style = AppInstaller::Settings::VisualStyle::Accent; + std::string m_message; bool UseVT() const { return m_enableVT && m_style != AppInstaller::Settings::VisualStyle::NoVT; } // Applies the selected visual style. void ApplyStyle(size_t i, size_t max, bool foregroundOnly); + void ClearLine(); + private: bool m_enableVT = false; }; @@ -70,14 +75,10 @@ namespace AppInstaller::CLI::Execution void EndProgress(bool hideProgressWhenDone); - void SetStyle(AppInstaller::Settings::VisualStyle style) { m_style = style; } - private: std::atomic<bool> m_isVisible = false; uint64_t m_lastCurrent = 0; - void ClearLine(); - void ShowProgressNoVT(uint64_t current, uint64_t maximum, ProgressType type); void ShowProgressWithVT(uint64_t current, uint64_t maximum, ProgressType type); diff --git a/src/AppInstallerCLICore/ExecutionReporter.cpp b/src/AppInstallerCLICore/ExecutionReporter.cpp @@ -216,6 +216,19 @@ namespace AppInstaller::CLI::Execution } } + void Reporter::SetProgressMessage(std::string_view message) + { + if (m_spinner) + { + m_spinner->SetMessage(message); + } + + if (m_progressBar) + { + m_progressBar->SetMessage(message); + } + } + void Reporter::BeginProgress() { GetBasicOutputStream() << VirtualTerminal::Cursor::Visibility::DisableShow; @@ -229,6 +242,7 @@ namespace AppInstaller::CLI::Execution { m_progressBar->EndProgress(hideProgressWhenDone); } + SetProgressMessage({}); GetBasicOutputStream() << VirtualTerminal::Cursor::Visibility::EnableShow; }; diff --git a/src/AppInstallerCLICore/ExecutionReporter.h b/src/AppInstallerCLICore/ExecutionReporter.h @@ -113,6 +113,7 @@ namespace AppInstaller::CLI::Execution // IProgressSink void BeginProgress() override; void OnProgress(uint64_t current, uint64_t maximum, ProgressType type) override; + void SetProgressMessage(std::string_view message) override; void EndProgress(bool hideProgressWhenDone) override; // Runs the given callable of type: auto(IProgressCallback&) diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h @@ -209,6 +209,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(InstallFlowStartingPackageInstall); WINGET_DEFINE_RESOURCE_STRINGID(InstallLocationNotProvided); WINGET_DEFINE_RESOURCE_STRINGID(InstallScopeDescription); + WINGET_DEFINE_RESOURCE_STRINGID(InstallWaitingOnAnother); WINGET_DEFINE_RESOURCE_STRINGID(InteractiveArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(InvalidAliasError); WINGET_DEFINE_RESOURCE_STRINGID(InvalidArgumentSpecifierError); diff --git a/src/AppInstallerCLICore/TableOutput.h b/src/AppInstallerCLICore/TableOutput.h @@ -12,23 +12,6 @@ namespace AppInstaller::CLI::Execution { - namespace details - { - // Gets the column width of the console. - inline size_t GetConsoleWidth() - { - CONSOLE_SCREEN_BUFFER_INFO consoleInfo{}; - if (GetConsoleScreenBufferInfo(GetStdHandle(STD_OUTPUT_HANDLE), &consoleInfo)) - { - return static_cast<size_t>(consoleInfo.dwSize.X); - } - else - { - return 120; - } - } - } - // Enables output data in a table format. // TODO: Improve for use with sparse data. template <size_t FieldCount> @@ -144,7 +127,7 @@ namespace AppInstaller::CLI::Execution totalRequired += m_columns[i].MaxLength + (m_columns[i].SpaceAfter ? 1 : 0); } - size_t consoleWidth = details::GetConsoleWidth(); + size_t consoleWidth = GetConsoleWidth(); // If the total space would be too big, shrink them. // We don't want to use the last column, lest we auto-wrap diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -21,6 +21,7 @@ #include <winget/Archive.h> #include <Argument.h> #include <Command.h> +#include <AppInstallerSynchronization.h> using namespace winrt::Windows::ApplicationModel::Store::Preview::InstallControl; using namespace winrt::Windows::Foundation; @@ -151,6 +152,135 @@ namespace AppInstaller::CLI::Workflow }; } + namespace details + { + // Runs the installer via ShellExecute. + // Required Args: None + // Inputs: Installer, InstallerPath + // Outputs: None + void ShellExecuteInstall(Execution::Context& context) + { + context << + GetInstallerArgs << + ShellExecuteInstallImpl << + ReportInstallerResult("ShellExecute"sv, APPINSTALLER_CLI_ERROR_SHELLEXEC_INSTALL_FAILED); + } + + // Runs an MSI installer directly via MSI APIs. + // Required Args: None + // Inputs: Installer, InstallerPath + // Outputs: None + void DirectMSIInstall(Execution::Context& context) + { + context << + GetInstallerArgs << + DirectMSIInstallImpl << + ReportInstallerResult("MsiInstallProduct"sv, APPINSTALLER_CLI_ERROR_MSI_INSTALL_FAILED); + } + + // Deploys the MSIX. + // Required Args: None + // Inputs: Manifest?, Installer || InstallerPath + // Outputs: None + void MsixInstall(Execution::Context& context) + { + std::string uri; + if (context.Contains(Execution::Data::InstallerPath)) + { + uri = context.Get<Execution::Data::InstallerPath>().u8string(); + } + else + { + uri = context.Get<Execution::Data::Installer>()->Url; + } + + bool isMachineScope = Manifest::ConvertToScopeEnum(context.Args.GetArg(Execution::Args::Type::InstallScope)) == Manifest::ScopeEnum::Machine; + + // TODO: There was a bug in deployment api if provision api was called in packaged context. + // Remove this check when the OS bug is fixed and back ported. + if (isMachineScope && Runtime::IsRunningInPackagedContext()) + { + context.Reporter.Error() << Resource::String::InstallFlowReturnCodeSystemNotSupported << std::endl; + context.Add<Execution::Data::OperationReturnCode>(static_cast<DWORD>(APPINSTALLER_CLI_ERROR_INSTALL_SYSTEM_NOT_SUPPORTED)); + AICLI_LOG(CLI, Error, << "Device wide install for msix type is not supported in packaged context."); + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALL_SYSTEM_NOT_SUPPORTED); + } + + context.Reporter.Info() << Resource::String::InstallFlowStartingPackageInstall << std::endl; + + bool registrationDeferred = false; + + try + { + registrationDeferred = context.Reporter.ExecuteWithProgress([&](IProgressCallback& callback) + { + if (isMachineScope) + { + return Deployment::AddPackageMachineScope(uri, callback); + } + else + { + return Deployment::AddPackageWithDeferredFallback(uri, WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::InstallerTrusted), callback); + } + }); + } + catch (const wil::ResultException& re) + { + context.Add<Execution::Data::OperationReturnCode>(re.GetErrorCode()); + context << ReportInstallerResult("MSIX"sv, re.GetErrorCode(), /* isHResult */ true); + return; + } + + if (registrationDeferred) + { + context.Reporter.Warn() << Resource::String::InstallFlowRegistrationDeferred << std::endl; + } + else + { + context.Reporter.Info() << Resource::String::InstallFlowInstallSuccess << std::endl; + } + } + + // Runs the flow for installing a Portable package. + // Required Args: None + // Inputs: Installer, InstallerPath + // Outputs: None + void PortableInstall(Execution::Context& context) + { + context << + InitializePortableInstaller << + VerifyPackageAndSourceMatch << + PortableInstallImpl << + ReportInstallerResult("Portable"sv, APPINSTALLER_CLI_ERROR_PORTABLE_INSTALL_FAILED, true); + } + + // Runs the flow for installing a package from an archive. + // Required Args: None + // Inputs: Installer, InstallerPath, Manifest + // Outputs: None + void ArchiveInstall(Execution::Context& context) + { + context << + ScanArchiveFromLocalManifest << + ExtractFilesFromArchive << + VerifyAndSetNestedInstaller << + ExecuteInstallerForType(context.Get<Execution::Data::Installer>().value().NestedInstallerType); + } + } + + bool ExemptFromSingleInstallLocking(InstallerTypeEnum type) + { + switch (type) + { + // MSStore installs are always MSIX based; MSIX installs are safe to run in parallel. + case InstallerTypeEnum::Msix: + case InstallerTypeEnum::MSStore: + return true; + default: + return false; + } + } + void EnsureApplicableInstaller(Execution::Context& context) { const auto& installer = context.Get<Execution::Data::Installer>(); @@ -249,6 +379,21 @@ namespace AppInstaller::CLI::Workflow UpdateBehaviorEnum updateBehavior = context.Get<Execution::Data::Installer>().value().UpdateBehavior; bool doUninstallPrevious = isUpdate && (updateBehavior == UpdateBehaviorEnum::UninstallPrevious || context.Args.Contains(Execution::Args::Type::UninstallPrevious)); + Synchronization::CrossProcessInstallLock lock; + if (!ExemptFromSingleInstallLocking(m_installerType)) + { + // Acquire install lock; if the operation is cancelled it will return false so we will also return. + if (!context.Reporter.ExecuteWithProgress([&](IProgressCallback& callback) + { + callback.SetProgressMessage(Resource::String::InstallWaitingOnAnother()); + return lock.Acquire(callback); + })) + { + AICLI_LOG(CLI, Info, << "Abandoning attempt to acquire install lock due to cancellation"); + return; + } + } + switch (m_installerType) { case InstallerTypeEnum::Exe: @@ -266,15 +411,15 @@ namespace AppInstaller::CLI::Workflow } if (ShouldUseDirectMSIInstall(m_installerType, context.Args.Contains(Execution::Args::Type::Silent))) { - context << DirectMSIInstall; + context << details::DirectMSIInstall; } else { - context << ShellExecuteInstall; + context << details::ShellExecuteInstall; } break; case InstallerTypeEnum::Msix: - context << MsixInstall; + context << details::MsixInstall; break; case InstallerTypeEnum::MSStore: context << @@ -289,10 +434,10 @@ namespace AppInstaller::CLI::Workflow ExecuteUninstaller; context.ClearFlags(Execution::ContextFlag::InstallerExecutionUseUpdate); } - context << PortableInstall; + context << details::PortableInstall; break; case InstallerTypeEnum::Zip: - context << ArchiveInstall; + context << details::ArchiveInstall; break; default: THROW_HR(HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED)); @@ -319,99 +464,6 @@ namespace AppInstaller::CLI::Workflow context << Workflow::ExecuteInstallerForType(context.Get<Execution::Data::Installer>().value().BaseInstallerType); } - void ArchiveInstall(Execution::Context& context) - { - context << - ScanArchiveFromLocalManifest << - ExtractFilesFromArchive << - VerifyAndSetNestedInstaller << - ExecuteInstallerForType(context.Get<Execution::Data::Installer>().value().NestedInstallerType); - } - - void ShellExecuteInstall(Execution::Context& context) - { - context << - GetInstallerArgs << - ShellExecuteInstallImpl << - ReportInstallerResult("ShellExecute"sv, APPINSTALLER_CLI_ERROR_SHELLEXEC_INSTALL_FAILED); - } - - void DirectMSIInstall(Execution::Context& context) - { - context << - GetInstallerArgs << - DirectMSIInstallImpl << - ReportInstallerResult("MsiInstallProduct"sv, APPINSTALLER_CLI_ERROR_MSI_INSTALL_FAILED); - } - - void PortableInstall(Execution::Context& context) - { - context << - InitializePortableInstaller << - VerifyPackageAndSourceMatch << - PortableInstallImpl << - ReportInstallerResult("Portable"sv, APPINSTALLER_CLI_ERROR_PORTABLE_INSTALL_FAILED, true); - } - - void MsixInstall(Execution::Context& context) - { - std::string uri; - if (context.Contains(Execution::Data::InstallerPath)) - { - uri = context.Get<Execution::Data::InstallerPath>().u8string(); - } - else - { - uri = context.Get<Execution::Data::Installer>()->Url; - } - - bool isMachineScope = Manifest::ConvertToScopeEnum(context.Args.GetArg(Execution::Args::Type::InstallScope)) == Manifest::ScopeEnum::Machine; - - // TODO: There was a bug in deployment api if provision api was called in packaged context. - // Remove this check when the OS bug is fixed and back ported. - if (isMachineScope && Runtime::IsRunningInPackagedContext()) - { - context.Reporter.Error() << Resource::String::InstallFlowReturnCodeSystemNotSupported << std::endl; - context.Add<Execution::Data::OperationReturnCode>(static_cast<DWORD>(APPINSTALLER_CLI_ERROR_INSTALL_SYSTEM_NOT_SUPPORTED)); - AICLI_LOG(CLI, Error, << "Device wide install for msix type is not supported in packaged context."); - AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALL_SYSTEM_NOT_SUPPORTED); - } - - context.Reporter.Info() << Resource::String::InstallFlowStartingPackageInstall << std::endl; - - bool registrationDeferred = false; - - try - { - registrationDeferred = context.Reporter.ExecuteWithProgress([&](IProgressCallback& callback) - { - if (isMachineScope) - { - return Deployment::AddPackageMachineScope(uri, callback); - } - else - { - return Deployment::AddPackageWithDeferredFallback(uri, WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::InstallerTrusted), callback); - } - }); - } - catch (const wil::ResultException& re) - { - context.Add<Execution::Data::OperationReturnCode>(re.GetErrorCode()); - context << ReportInstallerResult("MSIX"sv, re.GetErrorCode(), /* isHResult */ true); - return; - } - - if (registrationDeferred) - { - context.Reporter.Warn() << Resource::String::InstallFlowRegistrationDeferred << std::endl; - } - else - { - context.Reporter.Info() << Resource::String::InstallFlowInstallSuccess << std::endl; - } - } - void ReportInstallerResult::operator()(Execution::Context& context) const { DWORD installResult = context.Get<Execution::Data::OperationReturnCode>(); diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.h b/src/AppInstallerCLICore/Workflows/InstallFlow.h @@ -2,6 +2,7 @@ // Licensed under the MIT License. #pragma once #include "ExecutionContext.h" +#include <winget/Manifest.h> namespace AppInstaller::CLI::Workflow { @@ -11,6 +12,45 @@ namespace AppInstaller::CLI::Workflow static constexpr std::string_view ARG_TOKEN_LOGPATH = "<LOGPATH>"sv; static constexpr std::string_view ARG_TOKEN_INSTALLPATH = "<INSTALLPATH>"sv; + // Determines if an installer type is allowed to install/uninstall in parallel. + bool ExemptFromSingleInstallLocking(AppInstaller::Manifest::InstallerTypeEnum type); + + namespace details + { + // These single type install flows should remain "internal" and only ExecuteInstallerForType should be used externally + // so that all installs can properly handle single install locking. + + // Runs the installer via ShellExecute. + // Required Args: None + // Inputs: Installer, InstallerPath + // Outputs: None + void ShellExecuteInstall(Execution::Context& context); + + // Runs an MSI installer directly via MSI APIs. + // Required Args: None + // Inputs: Installer, InstallerPath + // Outputs: None + void DirectMSIInstall(Execution::Context& context); + + // Deploys the MSIX. + // Required Args: None + // Inputs: Manifest?, Installer || InstallerPath + // Outputs: None + void MsixInstall(Execution::Context& context); + + // Runs the flow for installing a Portable package. + // Required Args: None + // Inputs: Installer, InstallerPath + // Outputs: None + void PortableInstall(Execution::Context& context); + + // Runs the flow for installing a package from an archive. + // Required Args: None + // Inputs: Installer, InstallerPath, Manifest + // Outputs: None + void ArchiveInstall(Execution::Context& context); + } + // Ensures that there is an applicable installer. // Required Args: None // Inputs: Installer @@ -61,36 +101,6 @@ namespace AppInstaller::CLI::Workflow Manifest::InstallerTypeEnum m_installerType; }; - // Runs the installer via ShellExecute. - // Required Args: None - // Inputs: Installer, InstallerPath - // Outputs: None - void ShellExecuteInstall(Execution::Context& context); - - // Runs an MSI installer directly via MSI APIs. - // Required Args: None - // Inputs: Installer, InstallerPath - // Outputs: None - void DirectMSIInstall(Execution::Context& context); - - // Deploys the MSIX. - // Required Args: None - // Inputs: Manifest?, Installer || InstallerPath - // Outputs: None - void MsixInstall(Execution::Context& context); - - // Runs the flow for installing a Portable package. - // Required Args: None - // Inputs: Installer, InstallerPath - // Outputs: None - void PortableInstall(Execution::Context& context); - - // Runs the flow for installing a package from an archive. - // Required Args: None - // Inputs: Installer, InstallerPath, Manifest - // Outputs: None - void ArchiveInstall(Execution::Context& context); - // Verifies parameters for install to ensure success. // Required Args: None // Inputs: diff --git a/src/AppInstallerCLICore/Workflows/UninstallFlow.cpp b/src/AppInstallerCLICore/Workflows/UninstallFlow.cpp @@ -1,71 +1,73 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. -#include "pch.h" -#include "UninstallFlow.h" -#include "WorkflowBase.h" -#include "DependenciesFlow.h" -#include "ShellExecuteInstallerHandler.h" -#include "AppInstallerMsixInfo.h" +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#include "pch.h" +#include "UninstallFlow.h" +#include "InstallFlow.h" +#include "WorkflowBase.h" +#include "DependenciesFlow.h" +#include "ShellExecuteInstallerHandler.h" +#include "AppInstallerMsixInfo.h" #include "PortableFlow.h" -#include <AppInstallerDeployment.h> - -using namespace AppInstaller::CLI::Execution; -using namespace AppInstaller::Manifest; -using namespace AppInstaller::Msix; -using namespace AppInstaller::Repository; -using namespace AppInstaller::Registry; -using namespace AppInstaller::CLI::Portable; -using namespace AppInstaller::Utility::literals; - -namespace AppInstaller::CLI::Workflow -{ - namespace - { - // Helper for RecordUninstall - struct UninstallCorrelatedSources - { - struct Item - { - Utility::LocIndString Identifier; - Source FromSource; - std::string SourceIdentifier; - }; - - void AddIfRemoteAndNotPresent(const std::shared_ptr<IPackageVersion>& packageVersion) - { - auto source = packageVersion->GetSource(); - const auto details = source.GetDetails(); - if (!source.ContainsAvailablePackages()) - { - return; - } - - for (const auto& item : Items) - { - if (item.SourceIdentifier == details.Identifier) - { - return; - } - } - - Items.emplace_back(Item{ packageVersion->GetProperty(PackageVersionProperty::Id), std::move(source), details.Identifier }); - } - - std::vector<Item> Items; - }; - } - +#include <AppInstallerDeployment.h> +#include <AppInstallerSynchronization.h> + +using namespace AppInstaller::CLI::Execution; +using namespace AppInstaller::Manifest; +using namespace AppInstaller::Msix; +using namespace AppInstaller::Repository; +using namespace AppInstaller::Registry; +using namespace AppInstaller::CLI::Portable; +using namespace AppInstaller::Utility::literals; + +namespace AppInstaller::CLI::Workflow +{ + namespace + { + // Helper for RecordUninstall + struct UninstallCorrelatedSources + { + struct Item + { + Utility::LocIndString Identifier; + Source FromSource; + std::string SourceIdentifier; + }; + + void AddIfRemoteAndNotPresent(const std::shared_ptr<IPackageVersion>& packageVersion) + { + auto source = packageVersion->GetSource(); + const auto details = source.GetDetails(); + if (!source.ContainsAvailablePackages()) + { + return; + } + + for (const auto& item : Items) + { + if (item.SourceIdentifier == details.Identifier) + { + return; + } + } + + Items.emplace_back(Item{ packageVersion->GetProperty(PackageVersionProperty::Id), std::move(source), details.Identifier }); + } + + std::vector<Item> Items; + }; + } + void UninstallSinglePackage(Execution::Context& context) { - context << - Workflow::GetInstalledPackageVersion << - Workflow::EnsureSupportForUninstall << - Workflow::GetUninstallInfo << - Workflow::GetDependenciesInfoForUninstall << - Workflow::ReportDependencies(Resource::String::UninstallCommandReportDependencies) << - Workflow::ReportExecutionStage(ExecutionStage::Execution) << - Workflow::ExecuteUninstaller << - Workflow::ReportExecutionStage(ExecutionStage::PostExecution) << + context << + Workflow::GetInstalledPackageVersion << + Workflow::EnsureSupportForUninstall << + Workflow::GetUninstallInfo << + Workflow::GetDependenciesInfoForUninstall << + Workflow::ReportDependencies(Resource::String::UninstallCommandReportDependencies) << + Workflow::ReportExecutionStage(ExecutionStage::Execution) << + Workflow::ExecuteUninstaller << + Workflow::ReportExecutionStage(ExecutionStage::PostExecution) << Workflow::RecordUninstall; } @@ -117,128 +119,145 @@ namespace AppInstaller::CLI::Workflow } } - void GetUninstallInfo(Execution::Context& context) - { - auto installedPackageVersion = context.Get<Execution::Data::InstalledPackageVersion>(); - const std::string installedTypeString = installedPackageVersion->GetMetadata()[PackageVersionMetadata::InstalledType]; - switch (ConvertToInstallerTypeEnum(installedTypeString)) - { - case InstallerTypeEnum::Exe: - case InstallerTypeEnum::Burn: - case InstallerTypeEnum::Inno: - case InstallerTypeEnum::Nullsoft: - { - IPackageVersion::Metadata packageMetadata = installedPackageVersion->GetMetadata(); - - // Default to silent unless it is not present or interactivity is requested - auto uninstallCommandItr = packageMetadata.find(PackageVersionMetadata::SilentUninstallCommand); - if (uninstallCommandItr == packageMetadata.end() || context.Args.Contains(Execution::Args::Type::Interactive)) - { - auto interactiveItr = packageMetadata.find(PackageVersionMetadata::StandardUninstallCommand); - if (interactiveItr != packageMetadata.end()) - { - uninstallCommandItr = interactiveItr; - } - } - - if (uninstallCommandItr == packageMetadata.end()) - { - context.Reporter.Error() << Resource::String::NoUninstallInfoFound << std::endl; - AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_NO_UNINSTALL_INFO_FOUND); - } - - context.Add<Execution::Data::UninstallString>(uninstallCommandItr->second); - break; - } - case InstallerTypeEnum::Msi: - case InstallerTypeEnum::Wix: - { - // Uninstall strings for MSI don't include UI level (/q) needed to avoid interactivity, - // so we handle them differently. - auto productCodes = installedPackageVersion->GetMultiProperty(PackageVersionMultiProperty::ProductCode); - if (productCodes.empty()) - { - context.Reporter.Error() << Resource::String::NoUninstallInfoFound << std::endl; - AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_NO_UNINSTALL_INFO_FOUND); - } - - context.Add<Execution::Data::ProductCodes>(std::move(productCodes)); - break; - } - case InstallerTypeEnum::Msix: - case InstallerTypeEnum::MSStore: - { - auto packageFamilyNames = installedPackageVersion->GetMultiProperty(PackageVersionMultiProperty::PackageFamilyName); - if (packageFamilyNames.empty()) - { - context.Reporter.Error() << Resource::String::NoUninstallInfoFound << std::endl; - AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_NO_UNINSTALL_INFO_FOUND); - } - - context.Add<Execution::Data::PackageFamilyNames>(packageFamilyNames); - break; - } - case InstallerTypeEnum::Portable: - { + void GetUninstallInfo(Execution::Context& context) + { + auto installedPackageVersion = context.Get<Execution::Data::InstalledPackageVersion>(); + const std::string installedTypeString = installedPackageVersion->GetMetadata()[PackageVersionMetadata::InstalledType]; + switch (ConvertToInstallerTypeEnum(installedTypeString)) + { + case InstallerTypeEnum::Exe: + case InstallerTypeEnum::Burn: + case InstallerTypeEnum::Inno: + case InstallerTypeEnum::Nullsoft: + { + IPackageVersion::Metadata packageMetadata = installedPackageVersion->GetMetadata(); + + // Default to silent unless it is not present or interactivity is requested + auto uninstallCommandItr = packageMetadata.find(PackageVersionMetadata::SilentUninstallCommand); + if (uninstallCommandItr == packageMetadata.end() || context.Args.Contains(Execution::Args::Type::Interactive)) + { + auto interactiveItr = packageMetadata.find(PackageVersionMetadata::StandardUninstallCommand); + if (interactiveItr != packageMetadata.end()) + { + uninstallCommandItr = interactiveItr; + } + } + + if (uninstallCommandItr == packageMetadata.end()) + { + context.Reporter.Error() << Resource::String::NoUninstallInfoFound << std::endl; + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_NO_UNINSTALL_INFO_FOUND); + } + + context.Add<Execution::Data::UninstallString>(uninstallCommandItr->second); + break; + } + case InstallerTypeEnum::Msi: + case InstallerTypeEnum::Wix: + { + // Uninstall strings for MSI don't include UI level (/q) needed to avoid interactivity, + // so we handle them differently. auto productCodes = installedPackageVersion->GetMultiProperty(PackageVersionMultiProperty::ProductCode); if (productCodes.empty()) { context.Reporter.Error() << Resource::String::NoUninstallInfoFound << std::endl; AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_NO_UNINSTALL_INFO_FOUND); - } - + } + + context.Add<Execution::Data::ProductCodes>(std::move(productCodes)); + break; + } + case InstallerTypeEnum::Msix: + case InstallerTypeEnum::MSStore: + { + auto packageFamilyNames = installedPackageVersion->GetMultiProperty(PackageVersionMultiProperty::PackageFamilyName); + if (packageFamilyNames.empty()) + { + context.Reporter.Error() << Resource::String::NoUninstallInfoFound << std::endl; + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_NO_UNINSTALL_INFO_FOUND); + } + + context.Add<Execution::Data::PackageFamilyNames>(packageFamilyNames); + break; + } + case InstallerTypeEnum::Portable: + { + auto productCodes = installedPackageVersion->GetMultiProperty(PackageVersionMultiProperty::ProductCode); + if (productCodes.empty()) + { + context.Reporter.Error() << Resource::String::NoUninstallInfoFound << std::endl; + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_NO_UNINSTALL_INFO_FOUND); + } + const std::string installedScope = context.Get<Execution::Data::InstalledPackageVersion>()->GetMetadata()[Repository::PackageVersionMetadata::InstalledScope]; - const std::string installedArch = context.Get<Execution::Data::InstalledPackageVersion>()->GetMetadata()[Repository::PackageVersionMetadata::InstalledArchitecture]; - + const std::string installedArch = context.Get<Execution::Data::InstalledPackageVersion>()->GetMetadata()[Repository::PackageVersionMetadata::InstalledArchitecture]; + PortableInstaller portableInstaller = PortableInstaller( Manifest::ConvertToScopeEnum(installedScope), Utility::ConvertToArchitectureEnum(installedArch), - productCodes[0]); - context.Add<Execution::Data::PortableInstaller>(std::move(portableInstaller)); - break; - } - default: - THROW_HR(HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED)); - } - } - - void ExecuteUninstaller(Execution::Context& context) - { - const std::string installedTypeString = context.Get<Execution::Data::InstalledPackageVersion>()->GetMetadata()[PackageVersionMetadata::InstalledType]; - switch (ConvertToInstallerTypeEnum(installedTypeString)) - { - case InstallerTypeEnum::Exe: - case InstallerTypeEnum::Burn: - case InstallerTypeEnum::Inno: - case InstallerTypeEnum::Nullsoft: - context << - Workflow::ShellExecuteUninstallImpl << - ReportUninstallerResult("UninstallString", APPINSTALLER_CLI_ERROR_EXEC_UNINSTALL_COMMAND_FAILED); - break; - case InstallerTypeEnum::Msi: - case InstallerTypeEnum::Wix: - context << - Workflow::ShellExecuteMsiExecUninstall << - ReportUninstallerResult("MsiExec", APPINSTALLER_CLI_ERROR_EXEC_UNINSTALL_COMMAND_FAILED); - break; - case InstallerTypeEnum::Msix: - case InstallerTypeEnum::MSStore: - context << Workflow::MsixUninstall; - break; - case InstallerTypeEnum::Portable: - context << - Workflow::PortableUninstallImpl << - ReportUninstallerResult("PortableUninstall"sv, APPINSTALLER_CLI_ERROR_PORTABLE_UNINSTALL_FAILED, true); - break; - default: - THROW_HR(HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED)); - } - } - - void MsixUninstall(Execution::Context& context) - { - bool isMachineScope = Manifest::ConvertToScopeEnum(context.Args.GetArg(Execution::Args::Type::InstallScope)) == Manifest::ScopeEnum::Machine; - + productCodes[0]); + context.Add<Execution::Data::PortableInstaller>(std::move(portableInstaller)); + break; + } + default: + THROW_HR(HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED)); + } + } + + void ExecuteUninstaller(Execution::Context& context) + { + const std::string installedTypeString = context.Get<Execution::Data::InstalledPackageVersion>()->GetMetadata()[PackageVersionMetadata::InstalledType]; + InstallerTypeEnum installerType = ConvertToInstallerTypeEnum(installedTypeString); + + Synchronization::CrossProcessInstallLock lock; + if (!ExemptFromSingleInstallLocking(installerType)) + { + // Acquire install lock; if the operation is cancelled it will return false so we will also return. + if (!context.Reporter.ExecuteWithProgress([&](IProgressCallback& callback) + { + callback.SetProgressMessage(Resource::String::InstallWaitingOnAnother()); + return lock.Acquire(callback); + })) + { + AICLI_LOG(CLI, Info, << "Abandoning attempt to acquire install lock due to cancellation"); + return; + } + } + + switch (installerType) + { + case InstallerTypeEnum::Exe: + case InstallerTypeEnum::Burn: + case InstallerTypeEnum::Inno: + case InstallerTypeEnum::Nullsoft: + context << + Workflow::ShellExecuteUninstallImpl << + ReportUninstallerResult("UninstallString", APPINSTALLER_CLI_ERROR_EXEC_UNINSTALL_COMMAND_FAILED); + break; + case InstallerTypeEnum::Msi: + case InstallerTypeEnum::Wix: + context << + Workflow::ShellExecuteMsiExecUninstall << + ReportUninstallerResult("MsiExec", APPINSTALLER_CLI_ERROR_EXEC_UNINSTALL_COMMAND_FAILED); + break; + case InstallerTypeEnum::Msix: + case InstallerTypeEnum::MSStore: + context << Workflow::MsixUninstall; + break; + case InstallerTypeEnum::Portable: + context << + Workflow::PortableUninstallImpl << + ReportUninstallerResult("PortableUninstall"sv, APPINSTALLER_CLI_ERROR_PORTABLE_UNINSTALL_FAILED, true); + break; + default: + THROW_HR(HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED)); + } + } + + void MsixUninstall(Execution::Context& context) + { + bool isMachineScope = Manifest::ConvertToScopeEnum(context.Args.GetArg(Execution::Args::Type::InstallScope)) == Manifest::ScopeEnum::Machine; + // TODO: There was a bug in deployment api if deprovision api was called in packaged context. // Remove this check when the OS bug is fixed and back ported. if (isMachineScope && Runtime::IsRunningInPackagedContext()) @@ -247,66 +266,66 @@ namespace AppInstaller::CLI::Workflow context.Add<Execution::Data::OperationReturnCode>(static_cast<DWORD>(APPINSTALLER_CLI_ERROR_INSTALL_SYSTEM_NOT_SUPPORTED)); AICLI_LOG(CLI, Error, << "Device wide uninstall for msix type is not supported in packaged context."); AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALL_SYSTEM_NOT_SUPPORTED); - } - - const auto& packageFamilyNames = context.Get<Execution::Data::PackageFamilyNames>(); - context.Reporter.Info() << Resource::String::UninstallFlowStartingPackageUninstall << std::endl; - - for (const auto& packageFamilyName : packageFamilyNames) - { - auto packageFullName = Msix::GetPackageFullNameFromFamilyName(packageFamilyName); - if (!packageFullName.has_value()) - { - AICLI_LOG(CLI, Warning, << "No package found with family name: " << packageFamilyName); - continue; - } - - AICLI_LOG(CLI, Info, << "Removing MSIX package: " << packageFullName.value()); - try - { - if (isMachineScope) - { - context.Reporter.ExecuteWithProgress(std::bind(Deployment::RemovePackageMachineScope, packageFamilyName, packageFullName.value(), std::placeholders::_1)); - } - else - { - context.Reporter.ExecuteWithProgress(std::bind(Deployment::RemovePackage, packageFullName.value(), winrt::Windows::Management::Deployment::RemovalOptions::None, std::placeholders::_1)); - } - } + } + + const auto& packageFamilyNames = context.Get<Execution::Data::PackageFamilyNames>(); + context.Reporter.Info() << Resource::String::UninstallFlowStartingPackageUninstall << std::endl; + + for (const auto& packageFamilyName : packageFamilyNames) + { + auto packageFullName = Msix::GetPackageFullNameFromFamilyName(packageFamilyName); + if (!packageFullName.has_value()) + { + AICLI_LOG(CLI, Warning, << "No package found with family name: " << packageFamilyName); + continue; + } + + AICLI_LOG(CLI, Info, << "Removing MSIX package: " << packageFullName.value()); + try + { + if (isMachineScope) + { + context.Reporter.ExecuteWithProgress(std::bind(Deployment::RemovePackageMachineScope, packageFamilyName, packageFullName.value(), std::placeholders::_1)); + } + else + { + context.Reporter.ExecuteWithProgress(std::bind(Deployment::RemovePackage, packageFullName.value(), winrt::Windows::Management::Deployment::RemovalOptions::None, std::placeholders::_1)); + } + } catch (const wil::ResultException& re) { context.Add<Execution::Data::OperationReturnCode>(re.GetErrorCode()); context << ReportUninstallerResult("MSIXUninstall"sv, re.GetErrorCode(), /* isHResult */ true); return; - } - } - - context.Reporter.Info() << Resource::String::UninstallFlowUninstallSuccess << std::endl; - } - - void RecordUninstall(Context& context) - { - // In order to report an uninstall to every correlated tracking catalog, we first need to find them all. - auto package = context.Get<Data::Package>(); - UninstallCorrelatedSources correlatedSources; - - // Start with the installed version - correlatedSources.AddIfRemoteAndNotPresent(package->GetInstalledVersion()); - - // Then look through all available versions - for (const auto& versionKey : package->GetAvailableVersionKeys()) - { - correlatedSources.AddIfRemoteAndNotPresent(package->GetAvailableVersion(versionKey)); - } - - // Finally record the uninstall for each found value - for (const auto& item : correlatedSources.Items) - { - auto trackingCatalog = item.FromSource.GetTrackingCatalog(); - trackingCatalog.RecordUninstall(item.Identifier); - } - } - + } + } + + context.Reporter.Info() << Resource::String::UninstallFlowUninstallSuccess << std::endl; + } + + void RecordUninstall(Context& context) + { + // In order to report an uninstall to every correlated tracking catalog, we first need to find them all. + auto package = context.Get<Data::Package>(); + UninstallCorrelatedSources correlatedSources; + + // Start with the installed version + correlatedSources.AddIfRemoteAndNotPresent(package->GetInstalledVersion()); + + // Then look through all available versions + for (const auto& versionKey : package->GetAvailableVersionKeys()) + { + correlatedSources.AddIfRemoteAndNotPresent(package->GetAvailableVersion(versionKey)); + } + + // Finally record the uninstall for each found value + for (const auto& item : correlatedSources.Items) + { + auto trackingCatalog = item.FromSource.GetTrackingCatalog(); + trackingCatalog.RecordUninstall(item.Identifier); + } + } + void ReportUninstallerResult::operator()(Execution::Context& context) const { DWORD uninstallResult = context.Get<Execution::Data::OperationReturnCode>(); @@ -347,25 +366,25 @@ namespace AppInstaller::CLI::Workflow } } - void EnsureSupportForUninstall(Execution::Context& context) - { - auto installedPackageVersion = context.Get<Execution::Data::InstalledPackageVersion>(); - const std::string installedTypeString = installedPackageVersion->GetMetadata()[PackageVersionMetadata::InstalledType]; - auto installedType = ConvertToInstallerTypeEnum(installedTypeString); - if (installedType == InstallerTypeEnum::Portable) - { - const std::string installedScope = installedPackageVersion->GetMetadata()[Repository::PackageVersionMetadata::InstalledScope]; - if (Manifest::ConvertToScopeEnum(installedScope) == Manifest::ScopeEnum::Machine) - { - context << EnsureRunningAsAdmin; - } - } - else if (installedType == InstallerTypeEnum::Msix) - { - if (Manifest::ConvertToScopeEnum(context.Args.GetArg(Execution::Args::Type::InstallScope)) == Manifest::ScopeEnum::Machine) - { - context << EnsureRunningAsAdmin; - } - } + void EnsureSupportForUninstall(Execution::Context& context) + { + auto installedPackageVersion = context.Get<Execution::Data::InstalledPackageVersion>(); + const std::string installedTypeString = installedPackageVersion->GetMetadata()[PackageVersionMetadata::InstalledType]; + auto installedType = ConvertToInstallerTypeEnum(installedTypeString); + if (installedType == InstallerTypeEnum::Portable) + { + const std::string installedScope = installedPackageVersion->GetMetadata()[Repository::PackageVersionMetadata::InstalledScope]; + if (Manifest::ConvertToScopeEnum(installedScope) == Manifest::ScopeEnum::Machine) + { + context << EnsureRunningAsAdmin; + } + } + else if (installedType == InstallerTypeEnum::Msix) + { + if (Manifest::ConvertToScopeEnum(context.Args.GetArg(Execution::Args::Type::InstallScope)) == Manifest::ScopeEnum::Machine) + { + context << EnsureRunningAsAdmin; + } + } } -} +} diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1821,4 +1821,7 @@ Please specify one of them using the --source option to proceed.</value> <value>Reboot required to fully enable the Windows Feature(s); proceeding due to --force</value> <comment>"Windows Feature(s)" is the name of the options Windows features setting.</comment> </data> + <data name="InstallWaitingOnAnother" xml:space="preserve"> + <value>Waiting for another install/uninstall to complete...</value> + </data> </root> \ No newline at end of file diff --git a/src/AppInstallerCLITests/InstallFlow.cpp b/src/AppInstallerCLITests/InstallFlow.cpp @@ -5,6 +5,7 @@ #include "TestHooks.h" #include <AppInstallerFileLogger.h> #include <AppInstallerStrings.h> +#include <AppInstallerSynchronization.h> #include <Commands/InstallCommand.h> #include <Commands/UninstallCommand.h> #include <winget/AdminSettings.h> @@ -1151,4 +1152,55 @@ TEST_CASE("InstallFlow_InstallMultiple_SearchFailed", "[InstallFlow][workflow][M INFO(installOutput.str()); REQUIRE_TERMINATED_WITH(context, APPINSTALLER_CLI_ERROR_NOT_ALL_QUERIES_FOUND_SINGLE); -}- \ No newline at end of file +} + +TEST_CASE("InstallFlow_InstallAcquiresLock", "[InstallFlow][workflow]") +{ + TestCommon::TempFile installResultPath("TestExeInstalled.txt"); + + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + OverrideForOpenSource(context, CreateTestSource({ TSR::TestQuery_ReturnOne }), true); + OverrideForShellExecute(context); + context.Args.AddArg(Execution::Args::Type::Query, TSR::TestQuery_ReturnOne.Query); + + wil::unique_event enteredShellExecute; + enteredShellExecute.create(); + wil::unique_event canLeaveShellExecute; + canLeaveShellExecute.create(); + AppInstaller::ProgressCallback progress; + + context.Override({ ShellExecuteInstallImpl, [&](TestContext& context) + { + enteredShellExecute.SetEvent(); + canLeaveShellExecute.wait(500); + ShellExecuteInstallImpl(context); + }}); + + { + std::thread otherThread([&]() { + InstallCommand install({}); + install.Execute(context); + }); + + REQUIRE(enteredShellExecute.wait(5000)); + + AppInstaller::Synchronization::CrossProcessInstallLock mainThreadLock; + REQUIRE(!mainThreadLock.TryAcquireNoWait()); + + canLeaveShellExecute.SetEvent(); + otherThread.join(); + } + + INFO(installOutput.str()); + + // Verify Installer is called and parameters are passed in. + REQUIRE(std::filesystem::exists(installResultPath.GetPath())); + std::ifstream installResultFile(installResultPath.GetPath()); + REQUIRE(installResultFile.is_open()); + std::string installResultStr; + std::getline(installResultFile, installResultStr); + REQUIRE(installResultStr.find("/custom") != std::string::npos); + REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); +} diff --git a/src/AppInstallerCLITests/Synchronization.cpp b/src/AppInstallerCLITests/Synchronization.cpp @@ -23,8 +23,7 @@ TEST_CASE("CPRWL_MultipleReaders", "[CrossProcessReaderWriteLock]") // In the event of bugs, we don't want to block the test waiting forever otherThread.detach(); - // Wait up to a second for the other thread to do one thing... - REQUIRE(signal.wait(1000)); + REQUIRE(signal.wait(500)); } TEST_CASE("CPRWL_WriterBlocksReader", "[CrossProcessReaderWriteLock]") @@ -44,11 +43,11 @@ TEST_CASE("CPRWL_WriterBlocksReader", "[CrossProcessReaderWriteLock]") // In the event of bugs, we don't want to block the test waiting forever otherThread.detach(); - REQUIRE(!signal.wait(1000)); + REQUIRE(!signal.wait(500)); } // Upon release of the writer, the other thread should signal - REQUIRE(signal.wait(1000)); + REQUIRE(signal.wait(500)); } TEST_CASE("CPRWL_ReaderBlocksWriter", "[CrossProcessReaderWriteLock]") @@ -68,11 +67,11 @@ TEST_CASE("CPRWL_ReaderBlocksWriter", "[CrossProcessReaderWriteLock]") // In the event of bugs, we don't want to block the test waiting forever otherThread.detach(); - REQUIRE(!signal.wait(1000)); + REQUIRE(!signal.wait(500)); } // Upon release of the writer, the other thread should signal - REQUIRE(signal.wait(1000)); + REQUIRE(signal.wait(500)); } TEST_CASE("CPRWL_WriterBlocksWriter", "[CrossProcessReaderWriteLock]") @@ -92,11 +91,11 @@ TEST_CASE("CPRWL_WriterBlocksWriter", "[CrossProcessReaderWriteLock]") // In the event of bugs, we don't want to block the test waiting forever otherThread.detach(); - REQUIRE(!signal.wait(1000)); + REQUIRE(!signal.wait(500)); } // Upon release of the writer, the other thread should signal - REQUIRE(signal.wait(1000)); + REQUIRE(signal.wait(500)); } TEST_CASE("CPRWL_CancelEndsWait", "[CrossProcessReaderWriteLock]") @@ -116,10 +115,65 @@ TEST_CASE("CPRWL_CancelEndsWait", "[CrossProcessReaderWriteLock]") // In the event of bugs, we don't want to block the test waiting forever otherThread.detach(); - REQUIRE(!signal.wait(1000)); + REQUIRE(!signal.wait(500)); progress.Cancel(); // Upon release of the writer, the other thread should signal - REQUIRE(signal.wait(1000)); + REQUIRE(signal.wait(500)); +} + +TEST_CASE("CPIL_BlocksOthers", "[CrossProcessInstallLock]") +{ + wil::unique_event signal; + signal.create(); + AppInstaller::ProgressCallback progress; + + { + CrossProcessInstallLock mainThreadLock; + mainThreadLock.Acquire(progress); + + std::thread otherThread([&signal, &progress]() { + CrossProcessInstallLock otherThreadLock; + otherThreadLock.Acquire(progress); + signal.SetEvent(); + }); + // In the event of bugs, we don't want to block the test waiting forever + otherThread.detach(); + + REQUIRE(!signal.wait(500)); + } + + // Upon release of the writer, the other thread should signal + REQUIRE(signal.wait(500)); +} + +TEST_CASE("CPIL_CancelEndsWait", "[CrossProcessInstallLock]") +{ + wil::unique_event signal; + signal.create(); + AppInstaller::ProgressCallback progress; + + CrossProcessInstallLock mainThreadLock; + mainThreadLock.Acquire(progress); + + std::optional<bool> otherThreadAcquireResult = std::nullopt; + + std::thread otherThread([&]() { + CrossProcessInstallLock otherThreadLock; + otherThreadAcquireResult = otherThreadLock.Acquire(progress); + signal.SetEvent(); + }); + // In the event of bugs, we don't want to block the test waiting forever + otherThread.detach(); + + REQUIRE(!signal.wait(500)); + + progress.Cancel(); + + // Upon release of the writer, the other thread should signal + REQUIRE(signal.wait(500)); + + REQUIRE(otherThreadAcquireResult.has_value()); + REQUIRE(!otherThreadAcquireResult.value()); } diff --git a/src/AppInstallerCLITests/TestCommon.cpp b/src/AppInstallerCLITests/TestCommon.cpp @@ -162,6 +162,10 @@ namespace TestCommon } } + void TestProgress::SetProgressMessage(std::string_view) + { + } + void TestProgress::BeginProgress() { } diff --git a/src/AppInstallerCLITests/TestCommon.h b/src/AppInstallerCLITests/TestCommon.h @@ -100,7 +100,9 @@ namespace TestCommon void BeginProgress() override; void OnProgress(uint64_t current, uint64_t maximum, AppInstaller::ProgressType type) override; - + + void SetProgressMessage(std::string_view message) override; + void EndProgress(bool) override; bool IsCancelled() override; diff --git a/src/AppInstallerCLITests/WorkflowCommon.cpp b/src/AppInstallerCLITests/WorkflowCommon.cpp @@ -565,7 +565,7 @@ namespace TestCommon void OverrideForPortableInstall(TestContext& context) { - context.Override({ PortableInstall, [](TestContext&) + context.Override({ Workflow::details::PortableInstall, [](TestContext&) { std::filesystem::path temp = std::filesystem::temp_directory_path(); temp /= "TestPortableInstalled.txt"; @@ -628,7 +628,7 @@ namespace TestCommon void OverrideForMSIX(TestContext& context) { - context.Override({ MsixInstall, [](TestContext& context) + context.Override({ Workflow::details::MsixInstall, [](TestContext& context) { std::filesystem::path temp = std::filesystem::temp_directory_path(); temp /= "TestMsixInstalled.txt"; diff --git a/src/AppInstallerCommonCore/Progress.cpp b/src/AppInstallerCommonCore/Progress.cpp @@ -27,6 +27,15 @@ namespace AppInstaller } } + void ProgressCallback::SetProgressMessage(std::string_view message) + { + IProgressSink* sink = GetSink(); + if (sink) + { + sink->SetProgressMessage(message); + } + } + void ProgressCallback::EndProgress(bool hideProgressWhenDone) { IProgressSink* sink = GetSink(); @@ -85,6 +94,11 @@ namespace AppInstaller m_baseCallback.OnProgress(m_rangeMin + (m_rangeMax - m_rangeMin) * current / maximum, m_globalMax, type); } + void PartialPercentProgressCallback::SetProgressMessage(std::string_view message) + { + m_baseCallback.SetProgressMessage(message); + } + void PartialPercentProgressCallback::EndProgress(bool) { THROW_HR(E_NOTIMPL); diff --git a/src/AppInstallerCommonCore/Public/AppInstallerProgress.h b/src/AppInstallerCommonCore/Public/AppInstallerProgress.h @@ -4,6 +4,7 @@ #include <wil/resource.h> #include <atomic> #include <functional> +#include <string_view> namespace AppInstaller { @@ -35,6 +36,9 @@ namespace AppInstaller // If maximum is 0, the maximum is unknown. virtual void OnProgress(uint64_t current, uint64_t maximum, ProgressType type) = 0; + // Sets a message for the current progress state. + virtual void SetProgressMessage(std::string_view message) = 0; + // Called as progress begins. virtual void BeginProgress() = 0; @@ -65,6 +69,8 @@ namespace AppInstaller void OnProgress(uint64_t current, uint64_t maximum, ProgressType type) override; + void SetProgressMessage(std::string_view message) override; + void EndProgress(bool hideProgressWhenDone) override; bool IsCancelled() override; @@ -90,6 +96,8 @@ namespace AppInstaller void OnProgress(uint64_t current, uint64_t maximum, ProgressType type) override; + void SetProgressMessage(std::string_view message) override; + void EndProgress(bool hideProgressWhenDone) override; [[nodiscard]] IProgressCallback::CancelFunctionRemoval SetCancellationFunction(std::function<void()>&& f) override; diff --git a/src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h b/src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h @@ -55,4 +55,28 @@ namespace AppInstaller::Synchronization std::vector<wil::unique_mutex> m_mutexesHeld; }; + + // This lock is used to prevent multiple winget related processes from attempting to install (or uninstall) at the same time. + struct CrossProcessInstallLock + { + CrossProcessInstallLock(); + + // Acquires the lock; cancellation is enabled via the progress object. + // Returns true when the lock is acquired and false if the wait is cancelled. + bool Acquire(IProgressCallback& progress); + + // Optionally release the lock before destroying the object. + void Release(); + + // Attempts to acquire the mutex without a wait. + // Returns true if it was able, false if not. + bool TryAcquireNoWait(); + + // Indicates whether the lock is held. + operator bool() const; + + private: + wil::unique_mutex m_mutex; + wil::mutex_release_scope_exit m_lock; + }; } diff --git a/src/AppInstallerCommonCore/Synchronization.cpp b/src/AppInstallerCommonCore/Synchronization.cpp @@ -20,6 +20,9 @@ namespace AppInstaller::Synchronization // Arbitrary limit that should not ever cause a problem (theoretically 1 per process) constexpr size_t s_CrossProcessReaderWriteLock_MaxReaders = 8; + // The amount of time that we wait in between checking for cancellation + constexpr std::chrono::milliseconds s_CrossProcessInstallLock_WaitLoopTime = 250ms; + namespace { wil::unique_mutex OpenControlMutex(const std::wstring& name) @@ -259,4 +262,48 @@ namespace AppInstaller::Synchronization return result; } + + CrossProcessInstallLock::CrossProcessInstallLock() + { + m_mutex.create(L"WinGetCrossProcessInstallLock", 0, SYNCHRONIZE); + } + + bool CrossProcessInstallLock::Acquire(IProgressCallback& progress) + { + while (!progress.IsCancelled()) + { + auto lock = m_mutex.acquire(nullptr, static_cast<DWORD>(std::chrono::duration_cast<std::chrono::milliseconds>(s_CrossProcessInstallLock_WaitLoopTime).count())); + + if (lock) + { + m_lock = std::move(lock); + return true; + } + } + + return false; + } + + void CrossProcessInstallLock::Release() + { + m_lock.reset(); + } + + bool CrossProcessInstallLock::TryAcquireNoWait() + { + auto lock = m_mutex.acquire(nullptr, 0); + + if (lock) + { + m_lock = std::move(lock); + return true; + } + + return false; + } + + CrossProcessInstallLock::operator bool() const + { + return static_cast<bool>(m_lock); + } }