commit 2536bde0f6e6f502fd0193e0580787ccd4960091
parent cab941a8c458ca8b8685bd359c504f3e15ef1d06
Author: sreadingMSFT <74242768+sreadingMSFT@users.noreply.github.com>
Date: Tue, 24 Aug 2021 02:01:02 -0700
Fix GetInstallProgress to return null directly when an install is not in progress for that package (#1385)
* Return null asyncoperation from GetInstallProgress
* Fix header.
* Remove double lock from contextorchestrator.
* Fix spelling expectations
Diffstat:
6 files changed, 152 insertions(+), 73 deletions(-)
diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt
@@ -70,6 +70,7 @@ contosainstaller
contoso
contractversion
count'th
+countof
countryregion
createmanifestmetadata
cstdint
diff --git a/src/AppInstallerCLICore/ContextOrchestrator.cpp b/src/AppInstallerCLICore/ContextOrchestrator.cpp
@@ -42,14 +42,21 @@ namespace AppInstaller::CLI::Execution
void ContextOrchestrator::EnqueueItem(std::shared_ptr<OrchestratorQueueItem> item)
{
- std::lock_guard<std::mutex> lock{ m_queueLock };
+ {
+ std::lock_guard<std::mutex> lockQueue{ m_queueLock };
- THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INSTALL_ALREADY_RUNNING), FindById(item->GetId()));
- m_queueItems.push_back(item);
+ THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INSTALL_ALREADY_RUNNING), FindById(item->GetId()));
+ m_queueItems.push_back(item);
+ }
// Add the package to the Installing source so that it can be queried using the ISource interface.
const auto& manifest = item->GetContext().Get<Execution::Data::Manifest>();
m_installingWriteableSource->AddPackageVersion(manifest, std::filesystem::path{ manifest.Id + '.' + manifest.Version });
+
+ {
+ std::lock_guard<std::mutex> lockQueue{ m_queueLock };
+ item->SetState(OrchestratorQueueItemState::Queued);
+ }
}
void ContextOrchestrator::EnqueueAndRunItem(std::shared_ptr<OrchestratorQueueItem> item)
@@ -62,7 +69,7 @@ namespace AppInstaller::CLI::Execution
std::shared_ptr<OrchestratorQueueItem> ContextOrchestrator::GetNextItem()
{
- std::lock_guard<std::mutex> lock{ m_queueLock };
+ std::lock_guard<std::mutex> lockQueue{ m_queueLock };
if (m_queueItems.empty())
{
@@ -73,10 +80,10 @@ namespace AppInstaller::CLI::Execution
// Check if item can be dequeued.
// Since only one item can be installed at a time currently the logic is very simple,
- // and can just check if the first item is already running. This logic will need to become
+ // and can just check if the first item is ready to run. This logic will need to become
// more complicated if multiple operation types (e.g. Download & Install) are added that can
// run simultaneously.
- if (item->GetState() == OrchestratorQueueItemState::Running)
+ if (item->GetState() != OrchestratorQueueItemState::Queued)
{
return {};
}
@@ -121,21 +128,32 @@ namespace AppInstaller::CLI::Execution
void ContextOrchestrator::RemoveItemInState(const OrchestratorQueueItem& item, OrchestratorQueueItemState state)
{
- std::lock_guard<std::mutex> lock{ m_queueLock };
+ // OrchestratorQueueItemState::Running items should only be removed by the thread that ran the item.
+ // Queued items can be removed by any thread.
+ // NotQueued items should not be removed since, if found in the queue, they are in the process of being queued by another thread.
+ bool foundItem = false;
- // Look for the item. It's ok if the item is not found since multiple listeners may try to remove the same item.
- //auto itr = std::find(m_queueItems.begin(), m_queueItems.end(), item);
- auto itr = FindIteratorById(item.GetId());
- if (itr != m_queueItems.end() && (*itr)->GetState() == state)
{
- m_queueItems.erase(itr);
+ std::lock_guard<std::mutex> lockQueue{ m_queueLock };
+ // Look for the item. It's ok if the item is not found since multiple listeners may try to remove the same item.
+ auto itr = FindIteratorById(item.GetId());
+ if (itr != m_queueItems.end() && (*itr)->GetState() == state)
+ {
+ foundItem = true;
+ m_queueItems.erase(itr);
+ }
+ }
+
+ if (foundItem)
+ {
const auto& manifest = item.GetContext().Get<Execution::Data::Manifest>();
m_installingWriteableSource->RemovePackageVersion(manifest, std::filesystem::path{ manifest.Id + '.' + manifest.Version });
item.GetCompletedEvent().SetEvent();
}
}
+
void ContextOrchestrator::CancelQueueItem(const OrchestratorQueueItem& item)
{
// Always cancel the item, even if it isn't running yet, to get the terminationHR set correctly.
diff --git a/src/AppInstallerCLICore/ContextOrchestrator.h b/src/AppInstallerCLICore/ContextOrchestrator.h
@@ -14,6 +14,7 @@ namespace AppInstaller::CLI::Execution
{
enum class OrchestratorQueueItemState
{
+ NotQueued,
Queued,
Running
};
@@ -40,7 +41,7 @@ namespace AppInstaller::CLI::Execution
const wil::unique_event& GetCompletedEvent() const { return m_completedEvent; }
const OrchestratorQueueItemId& GetId() const { return m_id; }
private:
- OrchestratorQueueItemState m_state = OrchestratorQueueItemState::Queued;
+ OrchestratorQueueItemState m_state = OrchestratorQueueItemState::NotQueued;
std::unique_ptr<COMContext> m_context;
wil::unique_event m_completedEvent{ wil::EventOptions::ManualReset };
OrchestratorQueueItemId m_id;
diff --git a/src/Microsoft.Management.Deployment/Helpers.cpp b/src/Microsoft.Management.Deployment/Helpers.cpp
@@ -13,7 +13,7 @@ using namespace std::string_view_literals;
namespace winrt::Microsoft::Management::Deployment::implementation
{
- std::optional<DWORD> GetCallerProcessId()
+ std::pair<HRESULT, DWORD> GetCallerProcessId()
{
RPC_STATUS rpcStatus = RPC_S_OK;
RPC_CALL_ATTRIBUTES callAttributes = {};
@@ -25,9 +25,9 @@ namespace winrt::Microsoft::Management::Deployment::implementation
!((rpcStatus == RPC_S_OK) && HandleToULong(callAttributes.ClientPID) == GetCurrentProcessId()))
{
DWORD callerProcessId = HandleToULong(callAttributes.ClientPID);
- return callerProcessId;
+ return { S_OK, callerProcessId };
}
- return {};
+ return { E_ACCESSDENIED, 0 };
}
std::wstring_view GetStringForCapability(Capability capability)
@@ -57,14 +57,14 @@ namespace winrt::Microsoft::Management::Deployment::implementation
HRESULT EnsureComCallerHasCapability(Capability requiredCapability)
{
- auto callerProcessId = GetCallerProcessId();
- RETURN_HR_IF(E_ACCESSDENIED, !callerProcessId.has_value());
- HRESULT hr = EnsureProcessHasCapability(requiredCapability, callerProcessId.value());
+ auto [hr, callerProcessId] = GetCallerProcessId();
+ RETURN_IF_FAILED(hr);
+ hr = EnsureProcessHasCapability(requiredCapability, callerProcessId);
// The Windows.Management.Deployment API has set the precedent that packageManagement is a superset of packageQuery
// and packageQuery does not need to be declared separately.
if (FAILED(hr) && requiredCapability == Capability::PackageQuery)
{
- return EnsureProcessHasCapability(Capability::PackageManagement, callerProcessId.value());
+ return EnsureProcessHasCapability(Capability::PackageManagement, callerProcessId);
}
return hr;
}
diff --git a/src/Microsoft.Management.Deployment/Helpers.h b/src/Microsoft.Management.Deployment/Helpers.h
@@ -10,6 +10,6 @@ namespace winrt::Microsoft::Management::Deployment::implementation
HRESULT EnsureProcessHasCapability(Capability requiredCapability, DWORD callerProcessId);
HRESULT EnsureComCallerHasCapability(Capability requiredCapability);
- std::optional<DWORD> GetCallerProcessId();
+ std::pair<HRESULT, DWORD> GetCallerProcessId();
std::wstring TryGetCallerProcessInfo(DWORD callerProcessId);
}
\ No newline at end of file
diff --git a/src/Microsoft.Management.Deployment/PackageManager.cpp b/src/Microsoft.Management.Deployment/PackageManager.cpp
@@ -233,7 +233,7 @@ namespace winrt::Microsoft::Management::Deployment::implementation
return {};
}
}
-
+
Microsoft::Management::Deployment::PackageVersionInfo GetPackageVersionInfo(winrt::Microsoft::Management::Deployment::CatalogPackage package, winrt::Microsoft::Management::Deployment::InstallOptions options)
{
Microsoft::Management::Deployment::PackageVersionInfo packageVersionInfo{ nullptr };
@@ -255,8 +255,8 @@ namespace winrt::Microsoft::Management::Deployment::implementation
}
std::unique_ptr<::AppInstaller::COMContext> CreateContextFromInstallOptions(
- winrt::Microsoft::Management::Deployment::CatalogPackage package,
- winrt::Microsoft::Management::Deployment::InstallOptions options,
+ winrt::Microsoft::Management::Deployment::CatalogPackage package,
+ winrt::Microsoft::Management::Deployment::InstallOptions options,
std::wstring callerProcessInfoString)
{
std::unique_ptr<::AppInstaller::COMContext> context = std::make_unique<::AppInstaller::COMContext>();
@@ -366,45 +366,29 @@ namespace winrt::Microsoft::Management::Deployment::implementation
}
return nullptr;
}
-
winrt::Windows::Foundation::IAsyncOperationWithProgress<winrt::Microsoft::Management::Deployment::InstallResult, winrt::Microsoft::Management::Deployment::InstallProgress> GetInstallOperation(
- bool addToQueue,
- winrt::Microsoft::Management::Deployment::CatalogPackage package,
- winrt::Microsoft::Management::Deployment::InstallOptions options,
- winrt::Microsoft::Management::Deployment::PackageCatalogInfo catalogInfo)
+ bool canCancelQueueItem,
+ std::shared_ptr<Execution::OrchestratorQueueItem> queueItemParam,
+ winrt::Microsoft::Management::Deployment::CatalogPackage package = nullptr,
+ winrt::Microsoft::Management::Deployment::InstallOptions options = nullptr,
+ std::wstring callerProcessInfoString = {})
{
winrt::hresult terminationHR = S_OK;
hstring correlationData = (options) ? options.CorrelationData() : L"";
::Workflow::ExecutionStage executionStage = ::Workflow::ExecutionStage::Initial;
- #define WINGET_RETURN_INSTALL_RESULT_IF(installResult, boolVal) { if(boolVal) { co_return installResult; }}
- #define WINGET_RETURN_INSTALL_RESULT_HR(hr) { WINGET_RETURN_INSTALL_RESULT_IF(GetInstallResult(executionStage, hr, correlationData, false), true) }
- #define WINGET_RETURN_INSTALL_RESULT_HR_IF(hr, boolVal) { if(boolVal) { WINGET_RETURN_INSTALL_RESULT_HR(hr) }}
- #define WINGET_RETURN_INSTALL_RESULT_HR_IF_FAILED(hr) { WINGET_RETURN_INSTALL_RESULT_HR_IF(hr, FAILED(hr)) }
-
- // options and catalog can both be null, package must be set.
- WINGET_RETURN_INSTALL_RESULT_HR_IF(APPINSTALLER_CLI_ERROR_INVALID_CL_ARGUMENTS, !package);
-
try
{
+ // re-scope the parameter to inside the try block to avoid lifetime management issues.
+ std::shared_ptr<Execution::OrchestratorQueueItem> queueItem = std::move(queueItemParam);
+
auto report_progress{ co_await winrt::get_progress_token() };
auto cancellationToken{ co_await winrt::get_cancellation_token() };
+ // co_await does not guarantee that it's on a background thread, so do so explicitly.
+ co_await winrt::resume_background();
- wil::unique_event progressEvent{ wil::EventOptions::None };
-
- std::shared_ptr<Execution::OrchestratorQueueItem> queueItem = nullptr;
- if (addToQueue)
+ if (queueItem == nullptr)
{
- // Check for permissions and get caller info for telemetry.
- // This must be done before any co_awaits since it requires info from the rpc caller thread.
- std::optional<DWORD> callerProcessId = GetCallerProcessId();
- WINGET_RETURN_INSTALL_RESULT_HR_IF(E_ACCESSDENIED, !callerProcessId.has_value());
- WINGET_RETURN_INSTALL_RESULT_HR_IF_FAILED(EnsureProcessHasCapability(Capability::PackageManagement, callerProcessId.value()));
- std::wstring callerProcessInfoString = TryGetCallerProcessInfo(callerProcessId.value());
-
- // co_await does not guarantee that it's on a background thread, so do so explicitly.
- co_await winrt::resume_background();
-
Microsoft::Management::Deployment::PackageVersionInfo packageVersionInfo = GetPackageVersionInfo(package, options);
std::unique_ptr<::AppInstaller::COMContext> comContext = CreateContextFromInstallOptions(package, options, callerProcessInfoString);
queueItem = Execution::OrchestratorQueueItemFactory::CreateItemForInstall(std::wstring{ package.Id() }, std::wstring{ packageVersionInfo.PackageCatalog().Info().Id() }, std::move(comContext));
@@ -413,20 +397,14 @@ namespace winrt::Microsoft::Management::Deployment::implementation
InstallProgress queuedProgress{ PackageInstallProgressState::Queued, 0, 0, 0 };
report_progress(queuedProgress);
}
- else
{
- WINGET_RETURN_INSTALL_RESULT_HR_IF_FAILED(EnsureComCallerHasCapability(Capability::PackageQuery));
-
- queueItem = GetExistingQueueItemForPackage(package, catalogInfo);
- WINGET_RETURN_INSTALL_RESULT_IF(nullptr, queueItem == nullptr);
-
// correlation data is not passed in when retrieving an existing queue item, so get it from the existing context.
correlationData = hstring(queueItem->GetContext().GetCorrelationJson());
-
- // co_await does not guarantee that it's on a background thread, so do so explicitly.
- co_await winrt::resume_background();
}
+ wil::unique_event progressEvent{ wil::EventOptions::None };
+ wil::unique_event cancelWaitEvent{ wil::EventOptions::None };
+
std::atomic<winrt::Microsoft::Management::Deployment::InstallProgress> installProgress;
queueItem->GetContext().AddProgressCallbackFunction([&installProgress, &progressEvent](
::AppInstaller::ReportType reportType,
@@ -439,27 +417,41 @@ namespace winrt::Microsoft::Management::Deployment::implementation
if (installProgressOptional.has_value())
{
installProgress = installProgressOptional.value();
- ::SetEvent(progressEvent.get());
+ progressEvent.SetEvent();
}
return;
}
);
- cancellationToken.callback([&queueItem]
+
+ std::weak_ptr<Execution::OrchestratorQueueItem> weakQueueItem(queueItem);
+ cancellationToken.callback([weakQueueItem, &canCancelQueueItem, &cancelWaitEvent]
{
- // The cancellation of the AsyncOperation on the client triggers Cancel which causes the Execute to end.
- Execution::ContextOrchestrator::Instance().CancelQueueItem(*queueItem);
+ if (canCancelQueueItem)
+ {
+ auto strongQueueItem = weakQueueItem.lock();
+ if (strongQueueItem) {
+ // The cancellation of the AsyncOperation on the client triggers Cancel which causes the Execute to end.
+ Execution::ContextOrchestrator::Instance().CancelQueueItem(*strongQueueItem);
+ }
+ }
+ else
+ {
+ cancelWaitEvent.SetEvent();
+ }
});
// Wait for completion or progress events.
// Waiting for both on the same thread ensures that progress is never reported after the async operation itself has completed.
bool completionEventFired = false;
- HANDLE operationEvents[2];
+ bool cancelWaitEventFired = false;
+ HANDLE operationEvents[3];
operationEvents[0] = progressEvent.get();
operationEvents[1] = queueItem->GetCompletedEvent().get();
- while (!completionEventFired)
+ operationEvents[2] = cancelWaitEvent.get();
+ while (!completionEventFired && !cancelWaitEventFired)
{
DWORD dwEvent = WaitForMultipleObjects(
- 2 /* number of events */,
+ _countof(operationEvents) /* number of events */,
operationEvents /* event array */,
FALSE /* bWaitAll, FALSE to wake on any event */,
INFINITE /* wait until operation completion */);
@@ -481,30 +473,97 @@ namespace winrt::Microsoft::Management::Deployment::implementation
completionEventFired = true;
break;
+ // operationEvents[2] was signaled, operation is cancelled
+ case WAIT_OBJECT_0 + 2:
+ cancelWaitEventFired = true;
+ break;
+
// Return value is invalid.
default:
THROW_LAST_ERROR();
}
}
- // The install command has finished, check for success/failure and how far it got.
- terminationHR = queueItem->GetContext().GetTerminationHR();
- executionStage = queueItem->GetContext().GetExecutionStage();
+ if (completionEventFired)
+ {
+ // The install command has finished, check for success/failure and how far it got.
+ terminationHR = queueItem->GetContext().GetTerminationHR();
+ executionStage = queueItem->GetContext().GetExecutionStage();
+ }
}
WINGET_CATCH_STORE(terminationHR, APPINSTALLER_CLI_ERROR_COMMAND_FAILED);
// TODO - RebootRequired not yet populated, msi arguments not returned from Execute.
- WINGET_RETURN_INSTALL_RESULT_HR(terminationHR);
+ co_return GetInstallResult(executionStage, terminationHR, correlationData, false);
}
+ winrt::Windows::Foundation::IAsyncOperationWithProgress<winrt::Microsoft::Management::Deployment::InstallResult, winrt::Microsoft::Management::Deployment::InstallProgress> GetEmptyAsynchronousResultForInstallOperation(
+ HRESULT hr,
+ hstring correlationData)
+ {
+ // If a function uses co_await or co_return (i.e. if it is a co_routine), it cannot use return directly.
+ // This helper helps a function that is not a coroutine itself to return errors asynchronously.
+ co_return GetInstallResult(::Workflow::ExecutionStage::Initial, hr, correlationData, false);
+ }
+
+#define WINGET_RETURN_INSTALL_RESULT_HR_IF(hr, boolVal) { if(boolVal) { return GetEmptyAsynchronousResultForInstallOperation(hr, correlationData); }}
+#define WINGET_RETURN_INSTALL_RESULT_HR_IF_FAILED(hr) { WINGET_RETURN_INSTALL_RESULT_HR_IF(hr, FAILED(hr)) }
+
winrt::Windows::Foundation::IAsyncOperationWithProgress<winrt::Microsoft::Management::Deployment::InstallResult, winrt::Microsoft::Management::Deployment::InstallProgress> PackageManager::InstallPackageAsync(winrt::Microsoft::Management::Deployment::CatalogPackage package, winrt::Microsoft::Management::Deployment::InstallOptions options)
{
- return GetInstallOperation(true, package, options, nullptr);
+ hstring correlationData = (options) ? options.CorrelationData() : L"";
+
+ // options and catalog can both be null, package must be set.
+ WINGET_RETURN_INSTALL_RESULT_HR_IF(APPINSTALLER_CLI_ERROR_INVALID_CL_ARGUMENTS, !package);
+
+ HRESULT hr = S_OK;
+ std::wstring callerProcessInfoString;
+ try
+ {
+ // Check for permissions and get caller info for telemetry.
+ // This must be done before any co_awaits since it requires info from the rpc caller thread.
+ auto [hrGetCallerId, callerProcessId] = GetCallerProcessId();
+ WINGET_RETURN_INSTALL_RESULT_HR_IF_FAILED(hrGetCallerId);
+ WINGET_RETURN_INSTALL_RESULT_HR_IF_FAILED(EnsureProcessHasCapability(Capability::PackageManagement, callerProcessId));
+ callerProcessInfoString = TryGetCallerProcessInfo(callerProcessId);
+ }
+ WINGET_CATCH_STORE(hr, APPINSTALLER_CLI_ERROR_COMMAND_FAILED);
+ WINGET_RETURN_INSTALL_RESULT_HR_IF_FAILED(hr);
+
+ return GetInstallOperation(true /*canCancelQueueItem*/, nullptr /*queueItem*/, package, options, std::move(callerProcessInfoString));
}
winrt::Windows::Foundation::IAsyncOperationWithProgress<winrt::Microsoft::Management::Deployment::InstallResult, winrt::Microsoft::Management::Deployment::InstallProgress> PackageManager::GetInstallProgress(winrt::Microsoft::Management::Deployment::CatalogPackage package, winrt::Microsoft::Management::Deployment::PackageCatalogInfo catalogInfo)
{
- return GetInstallOperation(false, package, nullptr, catalogInfo);
+ hstring correlationData;
+ WINGET_RETURN_INSTALL_RESULT_HR_IF(APPINSTALLER_CLI_ERROR_INVALID_CL_ARGUMENTS, !package);
+
+ HRESULT hr = S_OK;
+ std::shared_ptr<Execution::OrchestratorQueueItem> queueItem = nullptr;
+ bool canCancelQueueItem = false;
+ try
+ {
+ // Check for permissions
+ // This must be done before any co_awaits since it requires info from the rpc caller thread.
+ auto [hrGetCallerId, callerProcessId] = GetCallerProcessId();
+ WINGET_RETURN_INSTALL_RESULT_HR_IF_FAILED(hrGetCallerId);
+ canCancelQueueItem = SUCCEEDED(EnsureProcessHasCapability(Capability::PackageManagement, callerProcessId));
+ if (!canCancelQueueItem)
+ {
+ WINGET_RETURN_INSTALL_RESULT_HR_IF_FAILED(EnsureProcessHasCapability(Capability::PackageQuery, callerProcessId));
+ }
+
+ // Get the queueItem synchronously.
+ queueItem = GetExistingQueueItemForPackage(package, catalogInfo);
+ if (queueItem == nullptr)
+ {
+ return nullptr;
+ }
+ }
+ WINGET_CATCH_STORE(hr, APPINSTALLER_CLI_ERROR_COMMAND_FAILED);
+ WINGET_RETURN_INSTALL_RESULT_HR_IF_FAILED(hr);
+
+ return GetInstallOperation(canCancelQueueItem, std::move(queueItem));
}
CoCreatableCppWinRtClass(PackageManager);
