commit b78cf8b7ea5639ef104baaa422f0372ef6f765c9 parent b94e128bdf773e185b87fce985d213c9d82ed742 Author: JohnMcPMS <johnmcp@microsoft.com> Date: Thu, 3 Jun 2021 16:21:55 -0700 Make CrossProcessReaderWriteLock resilient to process termination (#1098) The previous implementation (using a semaphore) was not resilient to unexpected process termination, as Windows does not track semaphore ownership. This could lead to a deadlock in a waiting process, and at least one user even experienced an ongoing deadlock despite having killed all of the active processes (which should have destroyed and thus reset the named semaphore). This version uses a set of named mutexes instead, which Windows will release for us in the event of abandonment. As part of the change I have also added the ability to pass in an `IProgressCallback` to allow for cancellation of the wait. On top of that change, I realized the the source management functions were not properly indicating that they were cancelled. While a CTRL+C in the middle of a `source add` would in fact cancel the action internally, it would still add the source information into the settings stream. I have updated the stack to handle cancellation now, including the output from top level now saying `Cancelled` rather than `Done` to indicate that CTRL+C actually did stop the action. Diffstat:
15 files changed, 457 insertions(+), 130 deletions(-)
diff --git a/.github/actions/spelling/allow.txt b/.github/actions/spelling/allow.txt @@ -257,6 +257,7 @@ msrc Multifile Multimatch mutex +mutexes namespace namespaces Nelon @@ -530,6 +531,7 @@ woah wofstream workaround workflow +wostringstream wostream wpfn wrl diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h @@ -27,6 +27,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(AvailableOptions); WINGET_DEFINE_RESOURCE_STRINGID(AvailableSubcommands); WINGET_DEFINE_RESOURCE_STRINGID(BothManifestAndSearchQueryProvided); + WINGET_DEFINE_RESOURCE_STRINGID(Cancelled); WINGET_DEFINE_RESOURCE_STRINGID(ChannelArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(Command); WINGET_DEFINE_RESOURCE_STRINGID(CommandArgumentDescription); diff --git a/src/AppInstallerCLICore/Workflows/SourceFlow.cpp b/src/AppInstallerCLICore/Workflows/SourceFlow.cpp @@ -94,9 +94,14 @@ namespace AppInstaller::CLI::Workflow Resource::String::SourceAddBegin << std::endl << " "_liv << name << " -> "_liv << arg << std::endl; - context.Reporter.ExecuteWithProgress(std::bind(Repository::AddSource, std::move(name), std::move(type), std::move(arg), std::placeholders::_1)); - - context.Reporter.Info() << Resource::String::Done; + if (context.Reporter.ExecuteWithProgress(std::bind(Repository::AddSource, std::move(name), std::move(type), std::move(arg), std::placeholders::_1))) + { + context.Reporter.Info() << Resource::String::Done; + } + else + { + context.Reporter.Info() << Resource::String::Cancelled << std::endl; + } } void ListSources(Execution::Context& context) @@ -160,8 +165,14 @@ namespace AppInstaller::CLI::Workflow for (const auto& sd : sources) { context.Reporter.Info() << Resource::String::SourceUpdateOne << ' ' << sd.Name << "..."_liv << std::endl; - context.Reporter.ExecuteWithProgress(std::bind(Repository::UpdateSource, sd.Name, std::placeholders::_1)); - context.Reporter.Info() << Resource::String::Done << std::endl; + if (context.Reporter.ExecuteWithProgress(std::bind(Repository::UpdateSource, sd.Name, std::placeholders::_1))) + { + context.Reporter.Info() << Resource::String::Done << std::endl; + } + else + { + context.Reporter.Info() << Resource::String::Cancelled << std::endl; + } } } @@ -178,8 +189,14 @@ namespace AppInstaller::CLI::Workflow for (const auto& sd : sources) { context.Reporter.Info() << Resource::String::SourceRemoveOne << ' ' << sd.Name << "..."_liv << std::endl; - context.Reporter.ExecuteWithProgress(std::bind(Repository::RemoveSource, sd.Name, std::placeholders::_1)); - context.Reporter.Info() << Resource::String::Done << std::endl; + if (context.Reporter.ExecuteWithProgress(std::bind(Repository::RemoveSource, sd.Name, std::placeholders::_1))) + { + context.Reporter.Info() << Resource::String::Done << std::endl; + } + else + { + context.Reporter.Info() << Resource::String::Cancelled << std::endl; + } } } diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -918,4 +918,7 @@ Configuration is disabled due to Group Policy.</value> <value>The value provided for the `%1` argument is invalid</value> <comment>{Locked="%1"} The value will be replaced with the argument name</comment> </data> + <data name="Cancelled" xml:space="preserve"> + <value>Cancelled</value> + </data> </root> \ No newline at end of file diff --git a/src/AppInstallerCLITests/Synchronization.cpp b/src/AppInstallerCLITests/Synchronization.cpp @@ -14,10 +14,10 @@ TEST_CASE("CPRWL_MultipleReaders", "[CrossProcessReaderWriteLock]") wil::unique_event signal; signal.create(); - CrossProcessReaderWriteLock mainThreadLock = CrossProcessReaderWriteLock::LockForRead(name); + CrossProcessReaderWriteLock mainThreadLock = CrossProcessReaderWriteLock::LockShared(name); std::thread otherThread([&name, &signal]() { - CrossProcessReaderWriteLock otherThreadLock = CrossProcessReaderWriteLock::LockForRead(name); + CrossProcessReaderWriteLock otherThreadLock = CrossProcessReaderWriteLock::LockShared(name); signal.SetEvent(); }); // In the event of bugs, we don't want to block the test waiting forever @@ -35,10 +35,10 @@ TEST_CASE("CPRWL_WriterBlocksReader", "[CrossProcessReaderWriteLock]") signal.create(); { - CrossProcessReaderWriteLock mainThreadLock = CrossProcessReaderWriteLock::LockForWrite(name); + CrossProcessReaderWriteLock mainThreadLock = CrossProcessReaderWriteLock::LockExclusive(name); std::thread otherThread([&name, &signal]() { - CrossProcessReaderWriteLock otherThreadLock = CrossProcessReaderWriteLock::LockForRead(name); + CrossProcessReaderWriteLock otherThreadLock = CrossProcessReaderWriteLock::LockShared(name); signal.SetEvent(); }); // In the event of bugs, we don't want to block the test waiting forever @@ -59,10 +59,10 @@ TEST_CASE("CPRWL_ReaderBlocksWriter", "[CrossProcessReaderWriteLock]") signal.create(); { - CrossProcessReaderWriteLock mainThreadLock = CrossProcessReaderWriteLock::LockForRead(name); + CrossProcessReaderWriteLock mainThreadLock = CrossProcessReaderWriteLock::LockShared(name); std::thread otherThread([&name, &signal]() { - CrossProcessReaderWriteLock otherThreadLock = CrossProcessReaderWriteLock::LockForWrite(name); + CrossProcessReaderWriteLock otherThreadLock = CrossProcessReaderWriteLock::LockExclusive(name); signal.SetEvent(); }); // In the event of bugs, we don't want to block the test waiting forever @@ -83,10 +83,10 @@ TEST_CASE("CPRWL_WriterBlocksWriter", "[CrossProcessReaderWriteLock]") signal.create(); { - CrossProcessReaderWriteLock mainThreadLock = CrossProcessReaderWriteLock::LockForWrite(name); + CrossProcessReaderWriteLock mainThreadLock = CrossProcessReaderWriteLock::LockExclusive(name); std::thread otherThread([&name, &signal]() { - CrossProcessReaderWriteLock otherThreadLock = CrossProcessReaderWriteLock::LockForWrite(name); + CrossProcessReaderWriteLock otherThreadLock = CrossProcessReaderWriteLock::LockExclusive(name); signal.SetEvent(); }); // In the event of bugs, we don't want to block the test waiting forever @@ -98,3 +98,28 @@ TEST_CASE("CPRWL_WriterBlocksWriter", "[CrossProcessReaderWriteLock]") // Upon release of the writer, the other thread should signal REQUIRE(signal.wait(1000)); } + +TEST_CASE("CPRWL_CancelEndsWait", "[CrossProcessReaderWriteLock]") +{ + std::string name = "AppInstCPRWLTests"; + + wil::unique_event signal; + signal.create(); + AppInstaller::ProgressCallback progress; + + CrossProcessReaderWriteLock mainThreadLock = CrossProcessReaderWriteLock::LockExclusive(name); + + std::thread otherThread([&name, &signal, &progress]() { + CrossProcessReaderWriteLock otherThreadLock = CrossProcessReaderWriteLock::LockExclusive(name, progress); + signal.SetEvent(); + }); + // In the event of bugs, we don't want to block the test waiting forever + otherThread.detach(); + + REQUIRE(!signal.wait(1000)); + + progress.Cancel(); + + // Upon release of the writer, the other thread should signal + REQUIRE(signal.wait(1000)); +} diff --git a/src/AppInstallerCLITests/TestSource.cpp b/src/AppInstallerCLITests/TestSource.cpp @@ -271,28 +271,31 @@ namespace TestCommon return OnCreate(details); } - void TestSourceFactory::Add(SourceDetails& details, IProgressCallback&) + bool TestSourceFactory::Add(SourceDetails& details, IProgressCallback&) { if (OnAdd) { OnAdd(details); } + return true; } - void TestSourceFactory::Update(const SourceDetails& details, IProgressCallback&) + bool TestSourceFactory::Update(const SourceDetails& details, IProgressCallback&) { if (OnUpdate) { OnUpdate(details); } + return true; } - void TestSourceFactory::Remove(const SourceDetails& details, IProgressCallback&) + bool TestSourceFactory::Remove(const SourceDetails& details, IProgressCallback&) { if (OnRemove) { OnRemove(details); } + return true; } // Make copies of self when requested. diff --git a/src/AppInstallerCLITests/TestSource.h b/src/AppInstallerCLITests/TestSource.h @@ -98,9 +98,9 @@ namespace TestCommon // ISourceFactory std::shared_ptr<AppInstaller::Repository::ISource> Create(const AppInstaller::Repository::SourceDetails& details, AppInstaller::IProgressCallback&) override; - void Add(AppInstaller::Repository::SourceDetails& details, AppInstaller::IProgressCallback&) override; - void Update(const AppInstaller::Repository::SourceDetails& details, AppInstaller::IProgressCallback&) override; - void Remove(const AppInstaller::Repository::SourceDetails& details, AppInstaller::IProgressCallback&) override; + bool Add(AppInstaller::Repository::SourceDetails& details, AppInstaller::IProgressCallback&) override; + bool Update(const AppInstaller::Repository::SourceDetails& details, AppInstaller::IProgressCallback&) override; + bool Remove(const AppInstaller::Repository::SourceDetails& details, AppInstaller::IProgressCallback&) override; // Make copies of self when requested. operator std::function<std::unique_ptr<AppInstaller::Repository::ISourceFactory>()>(); diff --git a/src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h b/src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h @@ -2,9 +2,14 @@ // Licensed under the MIT License. #pragma once #include <AppInstallerLanguageUtilities.h> +#include <AppInstallerProgress.h> #include <wil/resource.h> +#include <chrono> #include <string_view> +#include <vector> + +using namespace std::chrono_literals; namespace AppInstaller::Synchronization @@ -12,7 +17,7 @@ namespace AppInstaller::Synchronization // A fairly simple cross process (same session) reader-writer lock. // The primary purpose is for sources to control access to their backing stores. // Due to this design goal, these limitations exist: - // - Starves readers when a writer comes in. + // - Starves new readers when a writer comes in. // - Readers are limited to an arbitrarily chosen limit. // - Not re-entrant (although repeated read locking will work, it will consume additional slots). // - No upgrade from reader to writer. @@ -29,18 +34,18 @@ namespace AppInstaller::Synchronization CrossProcessReaderWriteLock(CrossProcessReaderWriteLock&&) = default; CrossProcessReaderWriteLock& operator=(CrossProcessReaderWriteLock&&) = default; - static CrossProcessReaderWriteLock LockForRead(std::string_view name); + static CrossProcessReaderWriteLock LockShared(std::string_view name); + static CrossProcessReaderWriteLock LockShared(std::string_view name, IProgressCallback& progress); - static CrossProcessReaderWriteLock LockForWrite(std::string_view name); + static CrossProcessReaderWriteLock LockExclusive(std::string_view name); + static CrossProcessReaderWriteLock LockExclusive(std::string_view name, IProgressCallback& progress); + static CrossProcessReaderWriteLock LockExclusive(std::string_view name, std::chrono::milliseconds timeout); - bool WasAbandoned() { return m_wasAbandoned; } + operator bool() const; private: - CrossProcessReaderWriteLock(std::string_view name); + static CrossProcessReaderWriteLock Lock(bool shared, std::string_view name, std::chrono::milliseconds timeout, IProgressCallback* progress); - wil::unique_mutex m_mutex; - wil::unique_semaphore m_semaphore; - ResetWhenMovedFrom<LONG> m_semaphoreReleases{ 0 }; - bool m_wasAbandoned = false; + std::vector<wil::unique_mutex> m_mutexesHeld; }; } diff --git a/src/AppInstallerCommonCore/Synchronization.cpp b/src/AppInstallerCommonCore/Synchronization.cpp @@ -11,66 +11,247 @@ namespace AppInstaller::Synchronization using namespace std::string_view_literals; constexpr std::wstring_view s_CrossProcessReaderWriteLock_MutexSuffix = L".mutex"sv; - constexpr std::wstring_view s_CrossProcessReaderWriteLock_SemaphoreSuffix = L".sem"sv; + + // A milliseconds version of INFINITE + constexpr std::chrono::milliseconds s_CrossProcessReaderWriteLock_Infinite = static_cast<std::chrono::milliseconds>(INFINITE); + + // The amount of time that we wait in between checking for cancellation + constexpr std::chrono::milliseconds s_CrossProcessReaderWriteLock_WaitLoopTime = 250ms; // Arbitrary limit that should not ever cause a problem (theoretically 1 per process) - constexpr LONG s_CrossProcessReaderWriteLock_MaxReaders = 16; + constexpr size_t s_CrossProcessReaderWriteLock_MaxReaders = 8; + + namespace + { + wil::unique_mutex OpenControlMutex(const std::wstring& name) + { + std::wstring mutexName = name; + mutexName += s_CrossProcessReaderWriteLock_MutexSuffix; + + wil::unique_mutex result; + result.create(mutexName.c_str(), 0, SYNCHRONIZE); + return result; + } + + wil::unique_mutex OpenAccessMutex(const std::wstring& name, size_t index) + { + THROW_HR_IF(E_INVALIDARG, index >= s_CrossProcessReaderWriteLock_MaxReaders); + std::wostringstream strstr; + strstr << name << L'.' << index; + + wil::unique_mutex result; + result.create(strstr.str().c_str(), 0, SYNCHRONIZE); + return result; + } + } CrossProcessReaderWriteLock::~CrossProcessReaderWriteLock() { - for (LONG i = 0; i < m_semaphoreReleases; ++i) + for (auto& mutex : m_mutexesHeld) { - m_semaphore.ReleaseSemaphore(); + ReleaseMutex(mutex.get()); } } - CrossProcessReaderWriteLock CrossProcessReaderWriteLock::LockForRead(std::string_view name) + CrossProcessReaderWriteLock CrossProcessReaderWriteLock::LockShared(std::string_view name) { - CrossProcessReaderWriteLock result(name); + return Lock(true, name, s_CrossProcessReaderWriteLock_Infinite, nullptr); + } - DWORD status = 0; - auto lock = result.m_mutex.acquire(&status); - THROW_HR_IF(E_UNEXPECTED, status != WAIT_OBJECT_0); + CrossProcessReaderWriteLock CrossProcessReaderWriteLock::LockShared(std::string_view name, IProgressCallback& progress) + { + return Lock(true, name, s_CrossProcessReaderWriteLock_Infinite, &progress); + } - // We are taking ownership of releasing this in the destructor - status = ::WaitForSingleObjectEx(result.m_semaphore.get(), INFINITE, FALSE); - THROW_HR_IF(E_UNEXPECTED, status != WAIT_OBJECT_0); + CrossProcessReaderWriteLock CrossProcessReaderWriteLock::LockExclusive(std::string_view name) + { + return Lock(false, name, s_CrossProcessReaderWriteLock_Infinite, nullptr); + } - result.m_semaphoreReleases = 1; - return result; + CrossProcessReaderWriteLock CrossProcessReaderWriteLock::LockExclusive(std::string_view name, IProgressCallback& progress) + { + return Lock(false, name, s_CrossProcessReaderWriteLock_Infinite, &progress); } - CrossProcessReaderWriteLock CrossProcessReaderWriteLock::LockForWrite(std::string_view name) + CrossProcessReaderWriteLock CrossProcessReaderWriteLock::LockExclusive(std::string_view name, std::chrono::milliseconds timeout) { - CrossProcessReaderWriteLock result(name); + return Lock(false, name, timeout, nullptr); + } + + CrossProcessReaderWriteLock::operator bool() const + { + return !m_mutexesHeld.empty(); + } + + CrossProcessReaderWriteLock CrossProcessReaderWriteLock::Lock( + bool shared, + std::string_view name, + std::chrono::milliseconds timeout, + IProgressCallback* progress) + { + auto start = std::chrono::steady_clock::now(); + + // Verify inputs + THROW_HR_IF(E_INVALIDARG, name.find('\\') != std::string::npos); + THROW_HR_IF(E_INVALIDARG, timeout.count() > INFINITE); + + CrossProcessReaderWriteLock result; + std::wstring wideName = Utility::ConvertToUTF16(name); + // Acquire overall control mutex DWORD status = 0; - auto lock = result.m_mutex.acquire(&status); - THROW_HR_IF_NULL(E_UNEXPECTED, lock); - result.m_wasAbandoned = (status == WAIT_ABANDONED); + wil::unique_mutex controlMutex = OpenControlMutex(wideName); + auto lock = controlMutex.acquire(&status, static_cast<DWORD>(timeout.count())); + THROW_LAST_ERROR_IF(status == WAIT_FAILED); - for (LONG i = 0; i < s_CrossProcessReaderWriteLock_MaxReaders; ++i) + if (status == WAIT_TIMEOUT || (progress && progress->IsCancelled())) { - // We are taking ownership of releasing these in the destructor - status = ::WaitForSingleObjectEx(result.m_semaphore.get(), INFINITE, FALSE); - THROW_HR_IF(E_UNEXPECTED, status != WAIT_OBJECT_0); - result.m_semaphoreReleases = i + 1; + return result; } - return result; - } + // Open all needed access mutexes + std::vector<wil::unique_mutex> allAccessMutexes; + HANDLE waitHandles[s_CrossProcessReaderWriteLock_MaxReaders]{}; - CrossProcessReaderWriteLock::CrossProcessReaderWriteLock(std::string_view name) - { - THROW_HR_IF(E_INVALIDARG, name.find('\\') != std::string::npos); + if (shared) + { + // Acquire the first access mutex we can find that is open, or all of them if needed. + // Use the process id as an arbitrary value in an attempt to reduce collisions + // while still allowing for re-entrance to not be arbitrary. + size_t offset = GetProcessId(GetCurrentProcess()) % s_CrossProcessReaderWriteLock_MaxReaders; - std::wstring mutexName = Utility::ConvertToUTF16(name); - std::wstring semName = mutexName; + for (size_t i = 0; i < s_CrossProcessReaderWriteLock_MaxReaders; ++i) + { + size_t index = (i + offset) % s_CrossProcessReaderWriteLock_MaxReaders; + + wil::unique_mutex current = OpenAccessMutex(wideName, index); + status = ::WaitForSingleObjectEx(current.get(), 0, FALSE); + + if (status == WAIT_OBJECT_0 || status == WAIT_ABANDONED) + { + // We found an empty one, continue on with it + result.m_mutexesHeld.emplace_back(std::move(current)); + return result; + } + else if (status == WAIT_TIMEOUT) + { + waitHandles[i] = current.get(); + allAccessMutexes.emplace_back(std::move(current)); + } + else + { + THROW_LAST_ERROR(); + } + } + } + else + { + // Open all of the access mutexes. + for (size_t i = 0; i < s_CrossProcessReaderWriteLock_MaxReaders; ++i) + { + wil::unique_mutex current = OpenAccessMutex(wideName, i); + waitHandles[i] = current.get(); + allAccessMutexes.emplace_back(std::move(current)); + } + } + + // Wait for one/all of the mutexes (or cancellation) + bool waitAgain = true; + while (waitAgain && (!progress || !progress->IsCancelled())) + { + DWORD millisecondsToWait = 0; + if (progress) + { + if (timeout == s_CrossProcessReaderWriteLock_Infinite) + { + millisecondsToWait = static_cast<DWORD>(s_CrossProcessReaderWriteLock_WaitLoopTime.count()); + } + else + { + auto currentDuration = std::chrono::steady_clock::now() - start; + if (currentDuration >= timeout) + { + // Allow an attempt to acquire with no wait + millisecondsToWait = 0; + waitAgain = false; + } + else + { + auto durationToWait = timeout - currentDuration; + if (durationToWait > s_CrossProcessReaderWriteLock_WaitLoopTime) + { + durationToWait = s_CrossProcessReaderWriteLock_WaitLoopTime; + } + else + { + waitAgain = false; + } + millisecondsToWait = static_cast<DWORD>(std::chrono::duration_cast<std::chrono::milliseconds>(durationToWait).count()); + } + } + } + else + { + // If there is no progress, we will do the full wait this time + waitAgain = false; - mutexName += s_CrossProcessReaderWriteLock_MutexSuffix; - semName += s_CrossProcessReaderWriteLock_SemaphoreSuffix; + if (timeout == s_CrossProcessReaderWriteLock_Infinite) + { + millisecondsToWait = INFINITE; + } + else + { + auto currentDuration = std::chrono::steady_clock::now() - start; + if (currentDuration >= timeout) + { + // Allow an attempt to acquire with no wait + millisecondsToWait = 0; + } + else + { + millisecondsToWait = static_cast<DWORD>(std::chrono::duration_cast<std::chrono::milliseconds>(timeout - currentDuration).count()); + } + } + } - m_mutex.create(mutexName.c_str(), 0, SYNCHRONIZE); - m_semaphore.create(s_CrossProcessReaderWriteLock_MaxReaders, s_CrossProcessReaderWriteLock_MaxReaders, semName.c_str(), SYNCHRONIZE | SEMAPHORE_MODIFY_STATE); + status = WaitForMultipleObjectsEx(s_CrossProcessReaderWriteLock_MaxReaders, waitHandles, (shared ? FALSE : TRUE), millisecondsToWait, FALSE); + THROW_LAST_ERROR_IF(status == WAIT_FAILED); + + if (status != WAIT_TIMEOUT) + { + break; + } + } + + if (status == WAIT_TIMEOUT || (progress && progress->IsCancelled())) + { + return result; + } + + if (shared) + { + size_t acquiredIndex = 0; + if (status >= WAIT_OBJECT_0 && status < (WAIT_OBJECT_0 + s_CrossProcessReaderWriteLock_MaxReaders)) + { + acquiredIndex = status - WAIT_OBJECT_0; + } + else if (status >= WAIT_ABANDONED_0 && status < (WAIT_ABANDONED_0 + s_CrossProcessReaderWriteLock_MaxReaders)) + { + acquiredIndex = status - WAIT_ABANDONED_0; + } + else + { + THROW_HR(E_UNEXPECTED); + } + + // Take the one that was acquired + result.m_mutexesHeld.emplace_back(std::move(allAccessMutexes[acquiredIndex])); + } + else + { + result.m_mutexesHeld = std::move(allAccessMutexes); + } + + return result; } } diff --git a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp @@ -54,13 +54,18 @@ namespace AppInstaller::Repository::Microsoft { THROW_HR_IF(E_INVALIDARG, details.Type != PreIndexedPackageSourceFactory::Type()); - auto lock = Synchronization::CrossProcessReaderWriteLock::LockForRead(CreateNameForCPRWL(details)); + auto lock = Synchronization::CrossProcessReaderWriteLock::LockShared(CreateNameForCPRWL(details), progress); + if (!lock) + { + return {}; + } + return CreateInternal(details, std::move(lock), progress); } virtual std::shared_ptr<ISource> CreateInternal(const SourceDetails& details, Synchronization::CrossProcessReaderWriteLock&& lock, IProgressCallback& progress) = 0; - void Add(SourceDetails& details, IProgressCallback& progress) override final + bool Add(SourceDetails& details, IProgressCallback& progress) override final { if (details.Type.empty()) { @@ -88,12 +93,56 @@ namespace AppInstaller::Repository::Microsoft details.Data = Msix::GetPackageFamilyNameFromFullName(fullName); details.Identifier = Msix::GetPackageFamilyNameFromFullName(fullName); - auto lock = Synchronization::CrossProcessReaderWriteLock::LockForWrite(CreateNameForCPRWL(details)); + auto lock = LockExclusive(details, progress); + if (!lock) + { + return false; + } - UpdateInternal(packageLocation, packageInfo, details, progress); + return UpdateInternal(packageLocation, packageInfo, details, progress); } - void Update(const SourceDetails& details, IProgressCallback& progress) override final + bool Update(const SourceDetails& details, IProgressCallback& progress) override final + { + return UpdateBase(details, false, progress); + } + + bool BackgroundUpdate(const SourceDetails& details, IProgressCallback& progress) override final + { + return UpdateBase(details, true, progress); + } + + virtual bool UpdateInternal(const std::string& packageLocation, Msix::MsixInfo& packageInfo, const SourceDetails& details, IProgressCallback& progress) = 0; + + bool Remove(const SourceDetails& details, IProgressCallback& progress) override final + { + THROW_HR_IF(E_INVALIDARG, details.Type != PreIndexedPackageSourceFactory::Type()); + auto lock = LockExclusive(details, progress); + if (!lock) + { + return false; + } + + return RemoveInternal(details, progress); + } + + virtual bool RemoveInternal(const SourceDetails& details, IProgressCallback&) = 0; + + private: + Synchronization::CrossProcessReaderWriteLock LockExclusive(const SourceDetails& details, IProgressCallback& progress, bool isBackground = false) + { + if (isBackground) + { + // If this is a background update, don't wait on the lock. + return Synchronization::CrossProcessReaderWriteLock::LockExclusive(CreateNameForCPRWL(details), 0ms); + } + else + { + return Synchronization::CrossProcessReaderWriteLock::LockExclusive(CreateNameForCPRWL(details), progress); + } + } + + bool UpdateBase(const SourceDetails& details, bool isBackground, IProgressCallback& progress) { THROW_HR_IF(E_INVALIDARG, details.Type != PreIndexedPackageSourceFactory::Type()); @@ -110,25 +159,17 @@ namespace AppInstaller::Repository::Microsoft if (progress.IsCancelled()) { AICLI_LOG(Repo, Info, << "Cancelling update upon request"); - return; + return false; } - auto lock = Synchronization::CrossProcessReaderWriteLock::LockForWrite(CreateNameForCPRWL(details)); - - UpdateInternal(packageLocation, packageInfo, details, progress); - } - - virtual void UpdateInternal(const std::string& packageLocation, Msix::MsixInfo& packageInfo, const SourceDetails& details, IProgressCallback& progress) = 0; - - void Remove(const SourceDetails& details, IProgressCallback& progress) override final - { - THROW_HR_IF(E_INVALIDARG, details.Type != PreIndexedPackageSourceFactory::Type()); - auto lock = Synchronization::CrossProcessReaderWriteLock::LockForWrite(CreateNameForCPRWL(details)); + auto lock = LockExclusive(details, progress, isBackground); + if (!lock) + { + return false; + } - RemoveInternal(details, progress); + return UpdateInternal(packageLocation, packageInfo, details, progress); } - - virtual void RemoveInternal(const SourceDetails& details, IProgressCallback&) = 0; }; // Source factory for running within a packaged context @@ -166,7 +207,7 @@ namespace AppInstaller::Repository::Microsoft return std::make_shared<SQLiteIndexSource>(details, GetPackageFamilyNameFromDetails(details), std::move(index), std::move(lock)); } - void UpdateInternal(const std::string& packageLocation, Msix::MsixInfo& packageInfo, const SourceDetails& details, IProgressCallback& progress) override + bool UpdateInternal(const std::string& packageLocation, Msix::MsixInfo& packageInfo, const SourceDetails& details, IProgressCallback& progress) override { // Check if the package is newer before calling into deployment. // This can save us a lot of time over letting deployment detect same version. @@ -176,14 +217,14 @@ namespace AppInstaller::Repository::Microsoft if (!packageInfo.IsNewerThan(extension->GetPackageVersion())) { AICLI_LOG(Repo, Info, << "Remote source data was not newer than existing, no update needed"); - return; + return true; } } if (progress.IsCancelled()) { AICLI_LOG(Repo, Info, << "Cancelling update upon request"); - return; + return false; } // Due to complications with deployment, download the file and deploy from @@ -239,9 +280,11 @@ namespace AppInstaller::Repository::Microsoft } } } + + return true; } - void RemoveInternal(const SourceDetails& details, IProgressCallback& callback) override + bool RemoveInternal(const SourceDetails& details, IProgressCallback& callback) override { auto fullName = Msix::GetPackageFullNameFromFamilyName(GetPackageFamilyNameFromDetails(details)); @@ -254,6 +297,8 @@ namespace AppInstaller::Repository::Microsoft AICLI_LOG(Repo, Info, << "Removing package: " << *fullName); Deployment::RemovePackage(*fullName, callback); } + + return true; } }; @@ -287,7 +332,7 @@ namespace AppInstaller::Repository::Microsoft return std::make_shared<SQLiteIndexSource>(details, GetPackageFamilyNameFromDetails(details), std::move(index), std::move(lock)); } - void UpdateInternal(const std::string&, Msix::MsixInfo& packageInfo, const SourceDetails& details, IProgressCallback& progress) override + bool UpdateInternal(const std::string&, Msix::MsixInfo& packageInfo, const SourceDetails& details, IProgressCallback& progress) override { // We will extract the manifest and index files directly to this location std::filesystem::path packageState = GetStatePathFromDetails(details); @@ -302,21 +347,23 @@ namespace AppInstaller::Repository::Microsoft if (!packageInfo.IsNewerThan(manifestPath)) { AICLI_LOG(Repo, Info, << "Remote source data was not newer than existing, no update needed"); - return; + return true; } } if (progress.IsCancelled()) { AICLI_LOG(Repo, Info, << "Cancelling update upon request"); - return; + return false; } packageInfo.WriteToFile(s_PreIndexedPackageSourceFactory_IndexFilePath, indexPath, progress); packageInfo.WriteManifestToFile(manifestPath, progress); + + return true; } - void RemoveInternal(const SourceDetails& details, IProgressCallback&) override + bool RemoveInternal(const SourceDetails& details, IProgressCallback&) override { std::filesystem::path packageState = GetStatePathFromDetails(details); @@ -329,6 +376,8 @@ namespace AppInstaller::Repository::Microsoft AICLI_LOG(Repo, Info, << "Removing state found for source: " << packageState.u8string()); std::filesystem::remove_all(packageState); } + + return true; } }; } diff --git a/src/AppInstallerRepositoryCore/Microsoft/PredefinedInstalledSourceFactory.cpp b/src/AppInstallerRepositoryCore/Microsoft/PredefinedInstalledSourceFactory.cpp @@ -131,19 +131,19 @@ namespace AppInstaller::Repository::Microsoft return std::make_shared<SQLiteIndexSource>(details, "*PredefinedInstalledSource", std::move(index), Synchronization::CrossProcessReaderWriteLock{}, true); } - void Add(SourceDetails&, IProgressCallback&) override final + bool Add(SourceDetails&, IProgressCallback&) override final { // Add should never be needed, as this is predefined. THROW_HR(E_NOTIMPL); } - void Update(const SourceDetails&, IProgressCallback&) override final + bool Update(const SourceDetails&, IProgressCallback&) override final { // Update could be used later, but not for now. THROW_HR(E_NOTIMPL); } - void Remove(const SourceDetails&, IProgressCallback&) override final + bool Remove(const SourceDetails&, IProgressCallback&) override final { // Similar to add, remove should never be needed. THROW_HR(E_NOTIMPL); diff --git a/src/AppInstallerRepositoryCore/Public/AppInstallerRepositorySource.h b/src/AppInstallerRepositoryCore/Public/AppInstallerRepositorySource.h @@ -102,7 +102,7 @@ namespace AppInstaller::Repository std::optional<SourceDetails> GetSource(std::string_view name); // Adds a new source for the user. - void AddSource(std::string_view name, std::string_view type, std::string_view arg, IProgressCallback& progress); + bool AddSource(std::string_view name, std::string_view type, std::string_view arg, IProgressCallback& progress); struct OpenSourceResult { diff --git a/src/AppInstallerRepositoryCore/RepositorySource.cpp b/src/AppInstallerRepositoryCore/RepositorySource.cpp @@ -590,16 +590,20 @@ namespace AppInstaller::Repository } template <typename MemberFunc> - void AddOrUpdateFromDetails(SourceDetails& details, MemberFunc member, IProgressCallback& progress) + bool AddOrUpdateFromDetails(SourceDetails& details, MemberFunc member, IProgressCallback& progress) { + bool result = false; auto factory = GetFactoryForType(details.Type); // Attempt; if it fails, wait a short time and retry. try { - (factory.get()->*member)(details, progress); - details.LastUpdateTime = std::chrono::system_clock::now(); - return; + result = (factory.get()->*member)(details, progress); + if (result) + { + details.LastUpdateTime = std::chrono::system_clock::now(); + } + return result; } CATCH_LOG(); @@ -607,25 +611,34 @@ namespace AppInstaller::Repository std::this_thread::sleep_for(2s); // If this one fails, maybe the problem is persistent. - (factory.get()->*member)(details, progress); - details.LastUpdateTime = std::chrono::system_clock::now(); + result = (factory.get()->*member)(details, progress); + if (result) + { + details.LastUpdateTime = std::chrono::system_clock::now(); + } + return result; + } + + bool AddSourceFromDetails(SourceDetails& details, IProgressCallback& progress) + { + return AddOrUpdateFromDetails(details, &ISourceFactory::Add, progress); } - void AddSourceFromDetails(SourceDetails& details, IProgressCallback& progress) + bool UpdateSourceFromDetails(SourceDetails& details, IProgressCallback& progress) { - AddOrUpdateFromDetails(details, &ISourceFactory::Add, progress); + return AddOrUpdateFromDetails(details, &ISourceFactory::Update, progress); } - void UpdateSourceFromDetails(SourceDetails& details, IProgressCallback& progress) + bool BackgroundUpdateSourceFromDetails(SourceDetails& details, IProgressCallback& progress) { - AddOrUpdateFromDetails(details, &ISourceFactory::Update, progress); + return AddOrUpdateFromDetails(details, &ISourceFactory::BackgroundUpdate, progress); } - void RemoveSourceFromDetails(const SourceDetails& details, IProgressCallback& progress) + bool RemoveSourceFromDetails(const SourceDetails& details, IProgressCallback& progress) { auto factory = GetFactoryForType(details.Type); - factory->Remove(details, progress); + return factory->Remove(details, progress); } // Determines whether (and logs why) a source should be updated before it is opened. @@ -847,7 +860,7 @@ namespace AppInstaller::Repository } } - void AddSource(std::string_view name, std::string_view type, std::string_view arg, IProgressCallback& progress) + bool AddSource(std::string_view name, std::string_view type, std::string_view arg, IProgressCallback& progress) { THROW_HR_IF(E_INVALIDARG, name.empty()); @@ -873,12 +886,16 @@ namespace AppInstaller::Repository details.LastUpdateTime = Utility::ConvertUnixEpochToSystemClock(0); details.Origin = SourceOrigin::User; - AddSourceFromDetails(details, progress); + bool result = AddSourceFromDetails(details, progress); + if (result) + { + AICLI_LOG(Repo, Info, << "Source created with extra data: " << details.Data); + AICLI_LOG(Repo, Info, << "Source created with identifier: " << details.Identifier); - AICLI_LOG(Repo, Info, << "Source created with extra data: " << details.Data); - AICLI_LOG(Repo, Info, << "Source created with identifier: " << details.Identifier); + sourceList.AddSource(details); + } - sourceList.AddSource(details); + return result; } OpenSourceResult OpenSource(std::string_view name, IProgressCallback& progress) @@ -915,8 +932,7 @@ namespace AppInstaller::Repository { // TODO: Consider adding a context callback to indicate we are doing the same action // to avoid the progress bar fill up multiple times. - UpdateSourceFromDetails(source, progress); - sourceUpdated = true; + sourceUpdated = BackgroundUpdateSourceFromDetails(source, progress) || sourceUpdated; } catch (...) { @@ -954,8 +970,10 @@ namespace AppInstaller::Repository { try { - UpdateSourceFromDetails(*source, progress); - sourceList.SaveMetadata(); + if (BackgroundUpdateSourceFromDetails(*source, progress)) + { + sourceList.SaveMetadata(); + } } catch (...) { @@ -1025,10 +1043,13 @@ namespace AppInstaller::Repository { AICLI_LOG(Repo, Info, << "Named source to be updated, found: " << source->Name); - UpdateSourceFromDetails(*source, progress); + bool result = UpdateSourceFromDetails(*source, progress); + if (result) + { + sourceList.SaveMetadata(); + } - sourceList.SaveMetadata(); - return true; + return result; } } @@ -1049,10 +1070,14 @@ namespace AppInstaller::Repository AICLI_LOG(Repo, Info, << "Named source to be removed, found: " << source->Name << " [" << ToString(source->Origin) << ']'); EnsureSourceIsRemovable(*source); - RemoveSourceFromDetails(*source, progress); - sourceList.RemoveSource(*source); - return true; + bool result = RemoveSourceFromDetails(*source, progress); + if (result) + { + sourceList.RemoveSource(*source); + } + + return result; } } diff --git a/src/AppInstallerRepositoryCore/Rest/RestSourceFactory.cpp b/src/AppInstallerRepositoryCore/Rest/RestSourceFactory.cpp @@ -24,7 +24,7 @@ namespace AppInstaller::Repository::Rest return std::make_shared<RestSource>(details, restClient.GetSourceIdentifier(), std::move(restClient)); } - void Add(SourceDetails& details, IProgressCallback&) override final + bool Add(SourceDetails& details, IProgressCallback&) override final { if (details.Type.empty()) { @@ -38,16 +38,20 @@ namespace AppInstaller::Repository::Rest // Check if URL is remote and secure THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_NOT_REMOTE, !Utility::IsUrlRemote(details.Arg)); THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_NOT_SECURE, !Utility::IsUrlSecure(details.Arg)); + + return true; } - void Update(const SourceDetails& details, IProgressCallback&) override final + bool Update(const SourceDetails& details, IProgressCallback&) override final { THROW_HR_IF(E_INVALIDARG, !Utility::CaseInsensitiveEquals(details.Type, RestSourceFactory::Type())); + return true; } - void Remove(const SourceDetails& details, IProgressCallback&) override final + bool Remove(const SourceDetails& details, IProgressCallback&) override final { THROW_HR_IF(E_INVALIDARG, !Utility::CaseInsensitiveEquals(details.Type, RestSourceFactory::Type())); + return true; } }; } diff --git a/src/AppInstallerRepositoryCore/SourceFactory.h b/src/AppInstallerRepositoryCore/SourceFactory.h @@ -18,12 +18,24 @@ namespace AppInstaller::Repository virtual std::shared_ptr<ISource> Create(const SourceDetails& details, IProgressCallback& progress) = 0; // Adds the source from the given details, writing back to the details any changes. - virtual void Add(SourceDetails& details, IProgressCallback& progress) = 0; + // Return value indicates whether the action completed. + virtual bool Add(SourceDetails& details, IProgressCallback& progress) = 0; // Updates the source from the given details (may not change the details). - virtual void Update(const SourceDetails& details, IProgressCallback& progress) = 0; + // Return value indicates whether the action completed. + virtual bool Update(const SourceDetails& details, IProgressCallback& progress) = 0; + + // Updates the source from the given details (may not change the details). + // This version is for use in automatic, background updates to the source. + // It is done this way to preserve the signature for use with member function pointers. + // Return value indicates whether the action completed. + virtual bool BackgroundUpdate(const SourceDetails& details, IProgressCallback& progress) + { + return Update(details, progress); + } // Removes the source from the given details. - virtual void Remove(const SourceDetails& details, IProgressCallback& progress) = 0; + // Return value indicates whether the action completed. + virtual bool Remove(const SourceDetails& details, IProgressCallback& progress) = 0; }; }