Skip to content

Commit

Permalink
Create cross process reader writer lock for source use (microsoft#41)
Browse files Browse the repository at this point in the history
  • Loading branch information
JohnMcPMS authored Feb 21, 2020
1 parent d6493dd commit 4a97e2c
Show file tree
Hide file tree
Showing 13 changed files with 243 additions and 15 deletions.
1 change: 1 addition & 0 deletions src/AppInstallerCLITests/AppInstallerCLITests.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
3 changes: 3 additions & 0 deletions src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters
Original file line number Diff line number Diff line change
Expand Up @@ -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" />
Expand Down
100 changes: 100 additions & 0 deletions src/AppInstallerCLITests/Synchronization.cpp
Original file line number Diff line number Diff line change
@@ -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));
}
4 changes: 3 additions & 1 deletion src/AppInstallerCLITests/pch.h
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -23,4 +25,4 @@
#include <vector>
#include <string>

#include <yaml-cpp/yaml.h>
#include <yaml-cpp/yaml.h>
18 changes: 10 additions & 8 deletions src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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>
Expand All @@ -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>
Expand All @@ -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" />
Expand All @@ -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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Expand Down Expand Up @@ -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" />
Expand Down
5 changes: 4 additions & 1 deletion src/AppInstallerCommonCore/FileLogger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 (...)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

#pragma once

#include <vector>
Expand Down
1 change: 0 additions & 1 deletion src/AppInstallerCommonCore/Public/AppInstallerDownloader.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

#pragma once

namespace AppInstaller::Utility
Expand Down
2 changes: 1 addition & 1 deletion src/AppInstallerCommonCore/Public/AppInstallerFileLogger.h
Original file line number Diff line number Diff line change
@@ -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>
Expand Down
2 changes: 0 additions & 2 deletions src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

#pragma once
#include "pch.h"

namespace AppInstaller::Msix
{
Expand Down
40 changes: 40 additions & 0 deletions src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h
Original file line number Diff line number Diff line change
@@ -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 };
};
}
75 changes: 75 additions & 0 deletions src/AppInstallerCommonCore/Synchronization.cpp
Original file line number Diff line number Diff line change
@@ -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);
}
}

0 comments on commit 4a97e2c

Please sign in to comment.