commit 4a97e2c1b67c3f9e9dc38d672a61049e2c65fb79 parent d6493dd2d27c61c888ab0f816c84c7bed6704954 Author: JohnMcPMS <johnmcp@microsoft.com> Date: Thu, 20 Feb 2020 18:06:41 -0800 Create cross process reader writer lock for source use (#41) Diffstat:
13 files changed, 243 insertions(+), 16 deletions(-)
diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj @@ -167,6 +167,7 @@ <ClCompile Include="Sources.cpp" /> <ClCompile Include="SQLiteIndex.cpp" /> <ClCompile Include="SQLiteWrapper.cpp" /> + <ClCompile Include="Synchronization.cpp" /> <ClCompile Include="TestCommon.cpp" /> <ClCompile Include="YamlManifest.cpp" /> </ItemGroup> diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters @@ -62,6 +62,9 @@ <ClCompile Include="Sources.cpp"> <Filter>Source Files</Filter> </ClCompile> + <ClCompile Include="Synchronization.cpp"> + <Filter>Source Files</Filter> + </ClCompile> </ItemGroup> <ItemGroup> <None Include="PropertySheet.props" /> diff --git a/src/AppInstallerCLITests/Synchronization.cpp b/src/AppInstallerCLITests/Synchronization.cpp @@ -0,0 +1,100 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#include "pch.h" +#include "TestCommon.h" + +#include <AppInstallerSynchronization.h> + +using namespace AppInstaller::Synchronization; + +TEST_CASE("CPRWL_MultipleReaders", "[CrossProcessReaderWriteLock]") +{ + std::string name = "AppInstCPRWLTests"; + + wil::unique_event signal; + signal.create(); + + CrossProcessReaderWriteLock mainThreadLock = CrossProcessReaderWriteLock::LockForRead(name); + + std::thread otherThread([&name, &signal]() { + CrossProcessReaderWriteLock otherThreadLock = CrossProcessReaderWriteLock::LockForRead(name); + signal.SetEvent(); + }); + // 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)); +} + +TEST_CASE("CPRWL_WriterBlocksReader", "[CrossProcessReaderWriteLock]") +{ + std::string name = "AppInstCPRWLTests"; + + wil::unique_event signal; + signal.create(); + + { + CrossProcessReaderWriteLock mainThreadLock = CrossProcessReaderWriteLock::LockForWrite(name); + + std::thread otherThread([&name, &signal]() { + CrossProcessReaderWriteLock otherThreadLock = CrossProcessReaderWriteLock::LockForRead(name); + signal.SetEvent(); + }); + // In the event of bugs, we don't want to block the test waiting forever + otherThread.detach(); + + REQUIRE(!signal.wait(1000)); + } + + // Upon release of the writer, the other thread should signal + REQUIRE(signal.wait(1000)); +} + +TEST_CASE("CPRWL_ReaderBlocksWriter", "[CrossProcessReaderWriteLock]") +{ + std::string name = "AppInstCPRWLTests"; + + wil::unique_event signal; + signal.create(); + + { + CrossProcessReaderWriteLock mainThreadLock = CrossProcessReaderWriteLock::LockForRead(name); + + std::thread otherThread([&name, &signal]() { + CrossProcessReaderWriteLock otherThreadLock = CrossProcessReaderWriteLock::LockForWrite(name); + signal.SetEvent(); + }); + // In the event of bugs, we don't want to block the test waiting forever + otherThread.detach(); + + REQUIRE(!signal.wait(1000)); + } + + // Upon release of the writer, the other thread should signal + REQUIRE(signal.wait(1000)); +} + +TEST_CASE("CPRWL_WriterBlocksWriter", "[CrossProcessReaderWriteLock]") +{ + std::string name = "AppInstCPRWLTests"; + + wil::unique_event signal; + signal.create(); + + { + CrossProcessReaderWriteLock mainThreadLock = CrossProcessReaderWriteLock::LockForWrite(name); + + std::thread otherThread([&name, &signal]() { + CrossProcessReaderWriteLock otherThreadLock = CrossProcessReaderWriteLock::LockForWrite(name); + signal.SetEvent(); + }); + // In the event of bugs, we don't want to block the test waiting forever + otherThread.detach(); + + REQUIRE(!signal.wait(1000)); + } + + // Upon release of the writer, the other thread should signal + REQUIRE(signal.wait(1000)); +} diff --git a/src/AppInstallerCLITests/pch.h b/src/AppInstallerCLITests/pch.h @@ -12,8 +12,10 @@ #include <winrt/Windows.Foundation.Collections.h> #include <winrt/Windows.Management.Deployment.h> +#include <wil/resource.h> #include <wil/result_macros.h> +#include <atomic> #include <filesystem> #include <fstream> #include <future> @@ -23,4 +25,4 @@ #include <vector> #include <string> -#include <yaml-cpp/yaml.h>- \ No newline at end of file +#include <yaml-cpp/yaml.h> diff --git a/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj b/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj @@ -122,9 +122,9 @@ <ClCompile> <Optimization>Disabled</Optimization> <PreprocessorDefinitions>_DEBUG;%(PreprocessorDefinitions);CLICOREDLLBUILD</PreprocessorDefinitions> - <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Debug|ARM'">$(ProjectDir);$(ProjectDir)Telemetry;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> - <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Debug|ARM64'">$(ProjectDir);$(ProjectDir)Telemetry;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> - <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">$(ProjectDir);$(ProjectDir)Telemetry;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> + <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Debug|ARM'">$(ProjectDir);$(ProjectDir)Public;$(ProjectDir)Telemetry;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> + <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Debug|ARM64'">$(ProjectDir);$(ProjectDir)Public;$(ProjectDir)Telemetry;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> + <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">$(ProjectDir);$(ProjectDir)Public;$(ProjectDir)Telemetry;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> <TreatWarningAsError Condition="'$(Configuration)|$(Platform)'=='Debug|ARM'">true</TreatWarningAsError> <TreatWarningAsError Condition="'$(Configuration)|$(Platform)'=='Debug|ARM64'">true</TreatWarningAsError> <TreatWarningAsError Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">true</TreatWarningAsError> @@ -139,7 +139,7 @@ <ItemDefinitionGroup Condition="'$(Platform)'=='Win32'"> <ClCompile> <PreprocessorDefinitions>WIN32;%(PreprocessorDefinitions);CLICOREDLLBUILD</PreprocessorDefinitions> - <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">$(ProjectDir);$(ProjectDir)Telemetry;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> + <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">$(ProjectDir);$(ProjectDir)Public;$(ProjectDir)Telemetry;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> <TreatWarningAsError Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">true</TreatWarningAsError> </ClCompile> <Link> @@ -152,10 +152,10 @@ <FunctionLevelLinking>true</FunctionLevelLinking> <IntrinsicFunctions>true</IntrinsicFunctions> <PreprocessorDefinitions>NDEBUG;%(PreprocessorDefinitions);CLICOREDLLBUILD</PreprocessorDefinitions> - <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Release|ARM'">$(ProjectDir);$(ProjectDir)Telemetry;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> - <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Release|ARM64'">$(ProjectDir);$(ProjectDir)Telemetry;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> - <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">$(ProjectDir);$(ProjectDir)Telemetry;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> - <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Release|x64'">$(ProjectDir);$(ProjectDir)Telemetry;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> + <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Release|ARM'">$(ProjectDir);$(ProjectDir)Public;$(ProjectDir)Telemetry;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> + <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Release|ARM64'">$(ProjectDir);$(ProjectDir)Public;$(ProjectDir)Telemetry;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> + <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">$(ProjectDir);$(ProjectDir)Public;$(ProjectDir)Telemetry;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> + <AdditionalIncludeDirectories Condition="'$(Configuration)|$(Platform)'=='Release|x64'">$(ProjectDir);$(ProjectDir)Public;$(ProjectDir)Telemetry;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> </ClCompile> <Link> <EnableCOMDATFolding>true</EnableCOMDATFolding> @@ -181,6 +181,7 @@ <ClInclude Include="Public\AppInstallerRuntime.h" /> <ClInclude Include="Public\AppInstallerSHA256.h" /> <ClInclude Include="Public\AppInstallerStrings.h" /> + <ClInclude Include="Public\AppInstallerSynchronization.h" /> <ClInclude Include="Public\AppInstallerTelemetry.h" /> <ClInclude Include="Public\AppInstallerLogging.h" /> <ClInclude Include="Public\AppInstallerArchitecture.h" /> @@ -204,6 +205,7 @@ </ClCompile> <ClCompile Include="AppInstallerTelemetry.cpp" /> <ClCompile Include="SHA256.cpp" /> + <ClCompile Include="Synchronization.cpp" /> <ClCompile Include="Telemetry\TraceLogging.cpp" /> <ClCompile Include="Architecture.cpp" /> </ItemGroup> diff --git a/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj.filters b/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj.filters @@ -81,6 +81,9 @@ <ClInclude Include="Public\AppInstallerDateTime.h"> <Filter>Public</Filter> </ClInclude> + <ClInclude Include="Public\AppInstallerSynchronization.h"> + <Filter>Public</Filter> + </ClInclude> </ItemGroup> <ItemGroup> <ClCompile Include="pch.cpp"> @@ -128,6 +131,9 @@ <ClCompile Include="MsixInfo.cpp"> <Filter>Source Files</Filter> </ClCompile> + <ClCompile Include="Synchronization.cpp"> + <Filter>Source Files</Filter> + </ClCompile> </ItemGroup> <ItemGroup> <None Include="PropertySheet.props" /> diff --git a/src/AppInstallerCommonCore/FileLogger.cpp b/src/AppInstallerCommonCore/FileLogger.cpp @@ -46,7 +46,10 @@ namespace AppInstaller::Logging void FileLogger::Write(Channel channel, Level, std::string_view message) noexcept try { - m_stream << std::chrono::system_clock::now() << " [" << std::setw(GetMaxChannelNameLength()) << std::left << std::setfill(' ') << GetChannelName(channel) << "] " << message << std::endl; + // Send to a string first to create a single block to write to a file. + std::stringstream strstr; + strstr << std::chrono::system_clock::now() << " [" << std::setw(GetMaxChannelNameLength()) << std::left << std::setfill(' ') << GetChannelName(channel) << "] " << message; + m_stream << strstr.str() << std::endl; } catch (...) { diff --git a/src/AppInstallerCommonCore/Public/AppInstallerArchitecture.h b/src/AppInstallerCommonCore/Public/AppInstallerArchitecture.h @@ -1,6 +1,5 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. - #pragma once #include <vector> diff --git a/src/AppInstallerCommonCore/Public/AppInstallerDownloader.h b/src/AppInstallerCommonCore/Public/AppInstallerDownloader.h @@ -1,6 +1,5 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. - #pragma once namespace AppInstaller::Utility diff --git a/src/AppInstallerCommonCore/Public/AppInstallerFileLogger.h b/src/AppInstallerCommonCore/Public/AppInstallerFileLogger.h @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #pragma once -#include "Public/AppInstallerLogging.h" +#include <AppInstallerLogging.h> #include <filesystem> #include <fstream> diff --git a/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h b/src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h @@ -1,8 +1,6 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. - #pragma once -#include "pch.h" namespace AppInstaller::Msix { diff --git a/src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h b/src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h @@ -0,0 +1,40 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#pragma once +#include <AppInstallerLanguageUtilities.h> +#include <wil/resource.h> + +#include <string_view> + + +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. + // - 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. + struct CrossProcessReaderWriteLock + { + ~CrossProcessReaderWriteLock(); + + CrossProcessReaderWriteLock(const CrossProcessReaderWriteLock&) = delete; + CrossProcessReaderWriteLock& operator=(const CrossProcessReaderWriteLock&) = delete; + + CrossProcessReaderWriteLock(CrossProcessReaderWriteLock&&) = default; + CrossProcessReaderWriteLock& operator=(CrossProcessReaderWriteLock&&) = default; + + static CrossProcessReaderWriteLock LockForRead(std::string_view name); + + static CrossProcessReaderWriteLock LockForWrite(std::string_view name); + + private: + CrossProcessReaderWriteLock(std::string_view name); + + wil::unique_mutex m_mutex; + wil::unique_semaphore m_semaphore; + ResetWhenMovedFrom<LONG> m_semaphoreReleases{ 0 }; + }; +} diff --git a/src/AppInstallerCommonCore/Synchronization.cpp b/src/AppInstallerCommonCore/Synchronization.cpp @@ -0,0 +1,75 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#pragma once +#include "pch.h" +#include <AppInstallerSynchronization.h> +#include <AppInstallerStrings.h> + + +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; + + // Arbitrary limit that should not ever cause a problem (theoretically 1 per process) + constexpr LONG s_CrossProcessReaderWriteLock_MaxReaders = 16; + + CrossProcessReaderWriteLock::~CrossProcessReaderWriteLock() + { + for (LONG i = 0; i < m_semaphoreReleases; ++i) + { + m_semaphore.ReleaseSemaphore(); + } + } + + CrossProcessReaderWriteLock CrossProcessReaderWriteLock::LockForRead(std::string_view name) + { + CrossProcessReaderWriteLock result(name); + + DWORD status = 0; + auto lock = result.m_mutex.acquire(&status); + THROW_HR_IF(E_UNEXPECTED, status != WAIT_OBJECT_0); + + // 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); + + result.m_semaphoreReleases = 1; + return result; + } + + CrossProcessReaderWriteLock CrossProcessReaderWriteLock::LockForWrite(std::string_view name) + { + CrossProcessReaderWriteLock result(name); + + DWORD status = 0; + auto lock = result.m_mutex.acquire(&status); + THROW_HR_IF(E_UNEXPECTED, status != WAIT_OBJECT_0); + + for (LONG i = 0; i < s_CrossProcessReaderWriteLock_MaxReaders; ++i) + { + // 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; + } + + CrossProcessReaderWriteLock::CrossProcessReaderWriteLock(std::string_view name) + { + THROW_HR_IF(E_INVALIDARG, name.find('\\') != std::string::npos); + + std::wstring mutexName = Utility::ConvertToUTF16(name); + std::wstring semName = mutexName; + + mutexName += s_CrossProcessReaderWriteLock_MutexSuffix; + semName += s_CrossProcessReaderWriteLock_SemaphoreSuffix; + + m_mutex.create(mutexName.c_str(), 0, SYNCHRONIZE); + m_semaphore.create(s_CrossProcessReaderWriteLock_MaxReaders, s_CrossProcessReaderWriteLock_MaxReaders, semName.c_str(), SYNCHRONIZE | SEMAPHORE_MODIFY_STATE); + } +}