commit d1083328e48f7b053042609fbba9f619c4048a02
parent 0c546572eb52b56f04f5368340be40128363989f
Author: JohnMcPMS <johnmcp@microsoft.com>
Date: Fri, 25 Aug 2023 11:36:22 -0700
Remove CrossProcessReaderWriteLock (#3549)
Remove `CrossProcessReaderWriteLock`, as it has caused more problems than it likely ever solved.
The current issue is that the mutexes can potentially be released on a different thread than they were acquired in the COM server. This led to initially removing them from being stored for the entire lifetime of the index file, after confirming that even in immutable mode, SQLite is opening the file and *not* sharing delete.
After that, the massive complexity of the code is unnecessary, since we ever only hold the lock shared to open the index. So rather than keep it, I swapped out all usage for the simple wrapper around a named mutex. I also added a `FAIL_FAST_IF` the current thread was not the one that we had originally acquired the lock on.
This might lead to slightly more waiting while other callers are opening files, but it might just as easily lead to less waiting due to the single mutex.
Diffstat:
9 files changed, 82 insertions(+), 458 deletions(-)
diff --git a/src/AppInstallerCLITests/Synchronization.cpp b/src/AppInstallerCLITests/Synchronization.cpp
@@ -7,122 +7,6 @@
using namespace AppInstaller::Synchronization;
-TEST_CASE("CPRWL_MultipleReaders", "[CrossProcessReaderWriteLock]")
-{
- std::string name = "AppInstCPRWLTests";
-
- wil::unique_event signal;
- signal.create();
-
- CrossProcessReaderWriteLock mainThreadLock = CrossProcessReaderWriteLock::LockShared(name);
-
- std::thread otherThread([&name, &signal]() {
- CrossProcessReaderWriteLock otherThreadLock = CrossProcessReaderWriteLock::LockShared(name);
- signal.SetEvent();
- });
- // In the event of bugs, we don't want to block the test waiting forever
- otherThread.detach();
-
- REQUIRE(signal.wait(500));
-}
-
-TEST_CASE("CPRWL_WriterBlocksReader", "[CrossProcessReaderWriteLock]")
-{
- std::string name = "AppInstCPRWLTests";
-
- wil::unique_event signal;
- signal.create();
-
- {
- CrossProcessReaderWriteLock mainThreadLock = CrossProcessReaderWriteLock::LockExclusive(name);
-
- std::thread otherThread([&name, &signal]() {
- CrossProcessReaderWriteLock otherThreadLock = CrossProcessReaderWriteLock::LockShared(name);
- 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("CPRWL_ReaderBlocksWriter", "[CrossProcessReaderWriteLock]")
-{
- std::string name = "AppInstCPRWLTests";
-
- wil::unique_event signal;
- signal.create();
-
- {
- CrossProcessReaderWriteLock mainThreadLock = CrossProcessReaderWriteLock::LockShared(name);
-
- std::thread otherThread([&name, &signal]() {
- CrossProcessReaderWriteLock otherThreadLock = CrossProcessReaderWriteLock::LockExclusive(name);
- 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("CPRWL_WriterBlocksWriter", "[CrossProcessReaderWriteLock]")
-{
- std::string name = "AppInstCPRWLTests";
-
- wil::unique_event signal;
- signal.create();
-
- {
- CrossProcessReaderWriteLock mainThreadLock = CrossProcessReaderWriteLock::LockExclusive(name);
-
- std::thread otherThread([&name, &signal]() {
- CrossProcessReaderWriteLock otherThreadLock = CrossProcessReaderWriteLock::LockExclusive(name);
- 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("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(500));
-
- progress.Cancel();
-
- // Upon release of the writer, the other thread should signal
- REQUIRE(signal.wait(500));
-}
-
TEST_CASE("CPIL_BlocksOthers", "[CrossProcessInstallLock]")
{
wil::unique_event signal;
diff --git a/src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h b/src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h
@@ -6,6 +6,7 @@
#include <wil/resource.h>
#include <chrono>
+#include <functional>
#include <string_view>
#include <vector>
@@ -14,52 +15,20 @@ using namespace std::chrono_literals;
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 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.
- // In order to change from reader/write, one must release and reacquire the lock:
- // auto lock = CrossProcessReaderWriteLock::LockShared(same_name);
- // ... Determine that an exclusive is needed ...
- // lock.Release();
- // lock = CrossProcessReaderWriteLock::LockExclusive(same_name);
- struct CrossProcessReaderWriteLock
+ // This is a standard named mutex.
+ // It must be acquired and released (or destroyed) on the same thread, just as all Windows mutexes must be.
+ struct CrossProcessLock
{
- // Create unheld lock.
- CrossProcessReaderWriteLock() = default;
+ CrossProcessLock(std::string_view name);
+ CrossProcessLock(const std::wstring& name);
- ~CrossProcessReaderWriteLock();
+ ~CrossProcessLock();
- CrossProcessReaderWriteLock(const CrossProcessReaderWriteLock&) = delete;
- CrossProcessReaderWriteLock& operator=(const CrossProcessReaderWriteLock&) = delete;
+ CrossProcessLock(const CrossProcessLock&) = delete;
+ CrossProcessLock& operator=(const CrossProcessLock&) = delete;
- CrossProcessReaderWriteLock(CrossProcessReaderWriteLock&&) = default;
- CrossProcessReaderWriteLock& operator=(CrossProcessReaderWriteLock&&) = default;
-
- static CrossProcessReaderWriteLock LockShared(std::string_view name);
- static CrossProcessReaderWriteLock LockShared(std::string_view name, IProgressCallback& progress);
-
- 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);
-
- operator bool() const;
-
- void Release();
-
- private:
- static CrossProcessReaderWriteLock Lock(bool shared, std::string_view name, std::chrono::milliseconds timeout, IProgressCallback* progress);
-
- 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();
+ CrossProcessLock(CrossProcessLock&&) = default;
+ CrossProcessLock& operator=(CrossProcessLock&&) = default;
// Acquires the lock; cancellation is enabled via the progress object.
// Returns true when the lock is acquired and false if the wait is cancelled.
@@ -78,5 +47,13 @@ namespace AppInstaller::Synchronization
private:
wil::unique_mutex m_mutex;
wil::mutex_release_scope_exit m_lock;
+ DWORD m_lockThreadId = 0;
+ };
+
+ // This lock is used to prevent multiple winget related processes from attempting to install (or uninstall) at the same time.
+ // It must be acquired and released (or destroyed) on the same thread, just as all Windows mutexes must be.
+ struct CrossProcessInstallLock : public CrossProcessLock
+ {
+ CrossProcessInstallLock() : CrossProcessLock(L"WinGetCrossProcessInstallLock") {}
};
}
diff --git a/src/AppInstallerCommonCore/Synchronization.cpp b/src/AppInstallerCommonCore/Synchronization.cpp
@@ -7,268 +7,24 @@
namespace AppInstaller::Synchronization
{
- using namespace std::string_view_literals;
-
- constexpr std::wstring_view s_CrossProcessReaderWriteLock_MutexSuffix = L".mutex"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 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)
- {
- 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()
- {
- Release();
- }
-
- CrossProcessReaderWriteLock CrossProcessReaderWriteLock::LockShared(std::string_view name)
- {
- return Lock(true, name, s_CrossProcessReaderWriteLock_Infinite, nullptr);
- }
-
- CrossProcessReaderWriteLock CrossProcessReaderWriteLock::LockShared(std::string_view name, IProgressCallback& progress)
+ CrossProcessLock::CrossProcessLock(std::string_view name) : CrossProcessLock(Utility::ConvertToUTF16(name))
{
- return Lock(true, name, s_CrossProcessReaderWriteLock_Infinite, &progress);
}
- CrossProcessReaderWriteLock CrossProcessReaderWriteLock::LockExclusive(std::string_view name)
+ CrossProcessLock::CrossProcessLock(const std::wstring& name)
{
- return Lock(false, name, s_CrossProcessReaderWriteLock_Infinite, nullptr);
- }
-
- CrossProcessReaderWriteLock CrossProcessReaderWriteLock::LockExclusive(std::string_view name, IProgressCallback& progress)
- {
- return Lock(false, name, s_CrossProcessReaderWriteLock_Infinite, &progress);
- }
-
- CrossProcessReaderWriteLock CrossProcessReaderWriteLock::LockExclusive(std::string_view name, std::chrono::milliseconds timeout)
- {
- return Lock(false, name, timeout, nullptr);
- }
-
- CrossProcessReaderWriteLock::operator bool() const
- {
- return !m_mutexesHeld.empty();
- }
-
- void CrossProcessReaderWriteLock::Release()
- {
- for (auto& mutex : m_mutexesHeld)
- {
- ReleaseMutex(mutex.get());
- }
- m_mutexesHeld.clear();
- }
-
- 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;
- wil::unique_mutex controlMutex = OpenControlMutex(wideName);
- auto lock = controlMutex.acquire(&status, static_cast<DWORD>(timeout.count()));
- THROW_LAST_ERROR_IF(status == WAIT_FAILED);
-
- if (status == WAIT_TIMEOUT || (progress && progress->IsCancelledBy(CancelReason::Any)))
- {
- return result;
- }
-
- // Open all needed access mutexes
- std::vector<wil::unique_mutex> allAccessMutexes;
- HANDLE waitHandles[s_CrossProcessReaderWriteLock_MaxReaders]{};
-
- 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;
-
- 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->IsCancelledBy(CancelReason::Any)))
- {
- 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;
-
- 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());
- }
- }
- }
-
- 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->IsCancelledBy(CancelReason::Any)))
- {
- 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;
+ m_mutex.create(name.c_str(), 0, SYNCHRONIZE);
}
- CrossProcessInstallLock::CrossProcessInstallLock()
+ CrossProcessLock::~CrossProcessLock()
{
- m_mutex.create(L"WinGetCrossProcessInstallLock", 0, SYNCHRONIZE);
+ Release();
}
- bool CrossProcessInstallLock::Acquire(IProgressCallback& progress)
+ bool CrossProcessLock::Acquire(IProgressCallback& progress)
{
while (!progress.IsCancelledBy(CancelReason::Any))
{
@@ -276,6 +32,7 @@ namespace AppInstaller::Synchronization
if (lock)
{
+ m_lockThreadId = GetCurrentThreadId();
m_lock = std::move(lock);
return true;
}
@@ -284,17 +41,24 @@ namespace AppInstaller::Synchronization
return false;
}
- void CrossProcessInstallLock::Release()
+ void CrossProcessLock::Release()
{
- m_lock.reset();
+ if (m_lock)
+ {
+ // Ensure that we are in fact always releasing on the same thread that acquired the lock.
+ // This is to force crashes rather than deadlocks in the event that we make a design error that leads to that.
+ FAIL_FAST_IF(m_lockThreadId != GetCurrentThreadId());
+ m_lock.reset();
+ }
}
- bool CrossProcessInstallLock::TryAcquireNoWait()
+ bool CrossProcessLock::TryAcquireNoWait()
{
auto lock = m_mutex.acquire(nullptr, 0);
if (lock)
{
+ m_lockThreadId = GetCurrentThreadId();
m_lock = std::move(lock);
return true;
}
@@ -302,7 +66,7 @@ namespace AppInstaller::Synchronization
return false;
}
- CrossProcessInstallLock::operator bool() const
+ CrossProcessLock::operator bool() const
{
return static_cast<bool>(m_lock);
}
diff --git a/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp b/src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp
@@ -106,10 +106,10 @@ namespace AppInstaller::Repository::Microsoft
}
// Creates a name for the cross process reader-writer lock given the details.
- std::string CreateNameForCPRWL(const SourceDetails& details)
+ std::string CreateNameForCPL(const SourceDetails& details)
{
// The only relevant data is the package family name
- return "PreIndexedSourceCPRWL_"s + GetPackageFamilyNameFromDetails(details);
+ return "PreIndexedSourceCPL_"s + GetPackageFamilyNameFromDetails(details);
}
// The base class for a package that comes from a preindexed packaged source.
@@ -194,17 +194,21 @@ namespace AppInstaller::Repository::Microsoft
virtual bool RemoveInternal(const SourceDetails& details, IProgressCallback&) = 0;
private:
- Synchronization::CrossProcessReaderWriteLock LockExclusive(const SourceDetails& details, IProgressCallback& progress, bool isBackground = false)
+ Synchronization::CrossProcessLock LockExclusive(const SourceDetails& details, IProgressCallback& progress, bool isBackground = false)
{
+ Synchronization::CrossProcessLock result(CreateNameForCPL(details));
+
if (isBackground)
{
// If this is a background update, don't wait on the lock.
- return Synchronization::CrossProcessReaderWriteLock::LockExclusive(CreateNameForCPRWL(details), 0ms);
+ result.TryAcquireNoWait();
}
else
{
- return Synchronization::CrossProcessReaderWriteLock::LockExclusive(CreateNameForCPRWL(details), progress);
+ result.Acquire(progress);
}
+
+ return result;
}
bool UpdateBase(const SourceDetails& details, bool isBackground, IProgressCallback& progress)
@@ -259,8 +263,8 @@ namespace AppInstaller::Repository::Microsoft
std::shared_ptr<ISource> Open(IProgressCallback& progress) override
{
- auto lock = Synchronization::CrossProcessReaderWriteLock::LockShared(CreateNameForCPRWL(m_details), progress);
- if (!lock)
+ Synchronization::CrossProcessLock lock(CreateNameForCPL(m_details));
+ if (!lock.Acquire(progress))
{
return {};
}
@@ -286,7 +290,7 @@ namespace AppInstaller::Repository::Microsoft
// We didn't use to store the source identifier, so we compute it here in case it's
// missing from the details.
m_details.Identifier = GetPackageFamilyNameFromDetails(m_details);
- return std::make_shared<SQLiteIndexSource>(m_details, std::move(index), std::move(lock), false, true);
+ return std::make_shared<SQLiteIndexSource>(m_details, std::move(index), false, true);
}
private:
@@ -421,8 +425,8 @@ namespace AppInstaller::Repository::Microsoft
std::shared_ptr<ISource> Open(IProgressCallback& progress) override
{
- auto lock = Synchronization::CrossProcessReaderWriteLock::LockShared(CreateNameForCPRWL(m_details), progress);
- if (!lock)
+ Synchronization::CrossProcessLock lock(CreateNameForCPL(m_details));
+ if (!lock.Acquire(progress))
{
return {};
}
@@ -461,7 +465,7 @@ namespace AppInstaller::Repository::Microsoft
// We didn't use to store the source identifier, so we compute it here in case it's
// missing from the details.
m_details.Identifier = GetPackageFamilyNameFromDetails(m_details);
- return std::make_shared<SQLiteIndexSource>(m_details, std::move(index), std::move(lock), false, true);
+ return std::make_shared<SQLiteIndexSource>(m_details, std::move(index), false, true);
}
private:
diff --git a/src/AppInstallerRepositoryCore/Microsoft/PredefinedInstalledSourceFactory.cpp b/src/AppInstallerRepositoryCore/Microsoft/PredefinedInstalledSourceFactory.cpp
@@ -158,7 +158,7 @@ namespace AppInstaller::Repository::Microsoft
PopulateIndexFromMSIX(index, Manifest::ScopeEnum::Machine);
}
- return std::make_shared<SQLiteIndexSource>(m_details, std::move(index), Synchronization::CrossProcessReaderWriteLock{}, true);
+ return std::make_shared<SQLiteIndexSource>(m_details, std::move(index), true);
}
private:
diff --git a/src/AppInstallerRepositoryCore/Microsoft/PredefinedWriteableSourceFactory.cpp b/src/AppInstallerRepositoryCore/Microsoft/PredefinedWriteableSourceFactory.cpp
@@ -68,7 +68,7 @@ namespace AppInstaller::Repository::Microsoft
// Create an in memory index without paths or dependencies
SQLiteIndex index = SQLiteIndex::CreateNew(SQLITE_MEMORY_DB_CONNECTION_TARGET, Schema::Version::Latest(), SQLiteIndex::CreateOptions::SupportPathless | SQLiteIndex::CreateOptions::DisableDependenciesSupport);
- g_sharedSource = std::make_shared<SQLiteIndexWriteableSource>(m_details, std::move(index), Synchronization::CrossProcessReaderWriteLock{}, true);
+ g_sharedSource = std::make_shared<SQLiteIndexWriteableSource>(m_details, std::move(index), true);
});
return g_sharedSource;
diff --git a/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndexSource.cpp b/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndexSource.cpp
@@ -396,10 +396,9 @@ namespace AppInstaller::Repository::Microsoft
SQLiteIndexSource::SQLiteIndexSource(
const SourceDetails& details,
SQLiteIndex&& index,
- Synchronization::CrossProcessReaderWriteLock&& lock,
bool isInstalledSource,
bool requireManifestHash) :
- m_details(details), m_lock(std::move(lock)), m_isInstalled(isInstalledSource), m_index(std::move(index)), m_requireManifestHash(requireManifestHash)
+ m_details(details), m_isInstalled(isInstalledSource), m_index(std::move(index)), m_requireManifestHash(requireManifestHash)
{
}
@@ -458,8 +457,8 @@ namespace AppInstaller::Repository::Microsoft
return const_cast<SQLiteIndexSource*>(this)->shared_from_this();
}
- SQLiteIndexWriteableSource::SQLiteIndexWriteableSource(const SourceDetails& details, SQLiteIndex&& index, Synchronization::CrossProcessReaderWriteLock&& lock, bool isInstalledSource) :
- SQLiteIndexSource(details, std::move(index), std::move(lock), isInstalledSource)
+ SQLiteIndexWriteableSource::SQLiteIndexWriteableSource(const SourceDetails& details, SQLiteIndex&& index, bool isInstalledSource) :
+ SQLiteIndexSource(details, std::move(index), isInstalledSource)
{
}
diff --git a/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndexSource.h b/src/AppInstallerRepositoryCore/Microsoft/SQLiteIndexSource.h
@@ -3,7 +3,6 @@
#pragma once
#include "Microsoft/SQLiteIndex.h"
#include "ISource.h"
-#include <AppInstallerSynchronization.h>
#include <memory>
@@ -18,7 +17,6 @@ namespace AppInstaller::Repository::Microsoft
SQLiteIndexSource(
const SourceDetails& details,
SQLiteIndex&& index,
- Synchronization::CrossProcessReaderWriteLock&& lock = {},
bool isInstalledSource = false,
bool requireManifestHash = false);
@@ -57,7 +55,6 @@ namespace AppInstaller::Repository::Microsoft
std::shared_ptr<SQLiteIndexSource> NonConstSharedFromThis() const;
SourceDetails m_details;
- Synchronization::CrossProcessReaderWriteLock m_lock;
bool m_requireManifestHash;
bool m_isInstalled;
@@ -71,7 +68,6 @@ namespace AppInstaller::Repository::Microsoft
SQLiteIndexWriteableSource(
const SourceDetails& details,
SQLiteIndex&& index,
- Synchronization::CrossProcessReaderWriteLock&& lock = {},
bool isInstalledSource = false);
// Casts to the requested type.
diff --git a/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp b/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp
@@ -18,9 +18,9 @@ namespace AppInstaller::Repository
{
constexpr std::string_view c_PackageTrackingFileName = "installed.db";
- std::string CreateNameForCPRWL(const std::string& pathName)
+ std::string CreateNameForCPL(const std::string& pathName)
{
- return "PackageTrackingCPRWL_"s + pathName;
+ return "PackageTrackingCPL_"s + pathName;
}
std::filesystem::path GetPackageTrackingFilePath(const std::string& pathName)
@@ -31,6 +31,22 @@ namespace AppInstaller::Repository
return result;
}
+ // Call while holding the CrossProcessLock
+ SQLiteIndex CreateOrOpenTrackingIndex(const std::filesystem::path& trackingDB)
+ {
+ if (!std::filesystem::exists(trackingDB))
+ {
+ std::filesystem::create_directories(trackingDB.parent_path());
+ return SQLiteIndex::CreateNew(trackingDB.u8string(), Schema::Version::Latest(), SQLiteIndex::CreateOptions::SupportPathless | SQLiteIndex::CreateOptions::DisableDependenciesSupport);
+ }
+ else
+ {
+ // TODO: Check schema version and upgrade as necessary when there is a relevant new schema.
+ // Could write this all now but it will be better tested when there is a new schema.
+ return SQLiteIndex::Open(trackingDB.u8string(), SQLiteIndex::OpenDisposition::ReadWrite);
+ }
+ }
+
struct PackageTrackingCatalogSourceReference : public ISourceReference
{
PackageTrackingCatalogSourceReference(const SourceDetails& details) : m_details(details) {}
@@ -45,32 +61,18 @@ namespace AppInstaller::Repository
return m_details.Identifier;
}
- std::shared_ptr<ISource> Open(IProgressCallback&) override
+ std::shared_ptr<ISource> Open(IProgressCallback& callback) override
{
m_details.Arg = Utility::MakeSuitablePathPart(m_details.Data);
std::filesystem::path trackingDB = GetPackageTrackingFilePath(m_details.Arg);
- std::string lockName = CreateNameForCPRWL(m_details.Arg);
-
- if (!std::filesystem::exists(trackingDB))
+ Synchronization::CrossProcessLock lock(CreateNameForCPL(m_details.Arg));
+ if (!lock.Acquire(callback))
{
- auto exclusiveLock = Synchronization::CrossProcessReaderWriteLock::LockExclusive(lockName);
-
- if (!std::filesystem::exists(trackingDB))
- {
- std::filesystem::create_directories(trackingDB.parent_path());
- SQLiteIndex::CreateNew(trackingDB.u8string(), Schema::Version::Latest(), SQLiteIndex::CreateOptions::SupportPathless | SQLiteIndex::CreateOptions::DisableDependenciesSupport);
- }
+ return {};
}
- auto lock = Synchronization::CrossProcessReaderWriteLock::LockShared(lockName);
-
- SQLiteIndex index = SQLiteIndex::Open(trackingDB.u8string(), SQLiteIndex::OpenDisposition::ReadWrite);
-
- // TODO: Check schema version and upgrade as necessary when there is a relevant new schema.
- // Could write this all now but it will be better tested when there is a new schema.
-
- return std::make_shared<SQLiteIndexSource>(m_details, std::move(index), std::move(lock));
+ return std::make_shared<SQLiteIndexSource>(m_details, CreateOrOpenTrackingIndex(trackingDB));
}
private:
@@ -108,10 +110,8 @@ namespace AppInstaller::Repository
std::string pathName = Utility::MakeSuitablePathPart(details.Data);
- std::string lockName = CreateNameForCPRWL(pathName);
- auto lock = Synchronization::CrossProcessReaderWriteLock::LockExclusive(lockName, progress);
-
- if (!lock)
+ Synchronization::CrossProcessLock lock(CreateNameForCPL(pathName));
+ if (!lock.Acquire(progress))
{
return false;
}
