Skip to content

Commit

Permalink
Settings streams exchange semantics (#1534)
Browse files Browse the repository at this point in the history 
The goal of this change is to add exchange semantics to settings stream writes.  Currently they do not have any interprocess synchronization.  This does not lead to corrupted streams as the underlying systems prevent simultaneous writes, but it can lead to data loss if updates from one process are not merged with those from another.

Because holding any kind of lock would likely lead to contention amongst the processes, we use exchange semantics instead.  Writes are only permitted if the stream has not changed since it was last read.  This required moving to a more object oriented model for the settings streams, which then required updating the consumers of it to also maintain an object and respond to failed write attempts.
  • Loading branch information
@JohnMcPMS
JohnMcPMS authored Oct 4, 2021
1 parent 5a97e26 commit fe8c617
Show file tree
Hide file tree
Showing 23 changed files with 1,807 additions and 1,083 deletions.
6 changes: 3 additions & 3 deletions src/AppInstallerCLITests/CustomHeader.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ TEST_CASE("RestClient_CustomHeader", "[RestSource][CustomHeader]")

TEST_CASE("AddSource_CustomHeader", "[RestSource][CustomHeader]")
{
SetSetting(Streams::UserSources, s_EmptySources);
SetSetting(Stream::UserSources, s_EmptySources);
TestHook_ClearSourceFactoryOverrides();

std::string customHeader = "Testing custom header with open source";
Expand All @@ -111,7 +111,7 @@ TEST_CASE("AddSource_CustomHeader", "[RestSource][CustomHeader]")

TEST_CASE("CreateSource_CustomHeader", "[RestSource][CustomHeader]")
{
SetSetting(Streams::UserSources, s_EmptySources);
SetSetting(Stream::UserSources, s_EmptySources);
TestHook_ClearSourceFactoryOverrides();

std::string customHeader = "Testing custom header with open source";
Expand All @@ -138,7 +138,7 @@ TEST_CASE("CreateSource_CustomHeader", "[RestSource][CustomHeader]")

TEST_CASE("CreateSource_CustomHeaderNotApplicable", "[RestSource][CustomHeader]")
{
SetSetting(Streams::UserSources, s_EmptySources);
SetSetting(Stream::UserSources, s_EmptySources);
TestHook_ClearSourceFactoryOverrides();

std::string customHeader = "Testing custom header with open source";
Expand Down
8 changes: 4 additions & 4 deletions src/AppInstallerCLITests/ExperimentalFeature.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,23 +39,23 @@ TEST_CASE("ExperimentalFeature ExperimentalCmd", "[experimentalFeature]")
SECTION("Feature on")
{
std::string_view json = R"({ "experimentalFeatures": { "experimentalCmd": true } })";
SetSetting(Streams::PrimaryUserSettings, json);
SetSetting(Stream::PrimaryUserSettings, json);
UserSettingsTest userSettingTest;

REQUIRE(ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::ExperimentalCmd, userSettingTest));
}
SECTION("Feature off")
{
std::string_view json = R"({ "experimentalFeatures": { "experimentalCmd": false } })";
SetSetting(Streams::PrimaryUserSettings, json);
SetSetting(Stream::PrimaryUserSettings, json);
UserSettingsTest userSettingTest;

REQUIRE_FALSE(ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::ExperimentalCmd, userSettingTest));
}
SECTION("Invalid value")
{
std::string_view json = R"({ "experimentalFeatures": { "experimentalCmd": "string" } })";
SetSetting(Streams::PrimaryUserSettings, json);
SetSetting(Stream::PrimaryUserSettings, json);
UserSettingsTest userSettingTest;

REQUIRE_FALSE(ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::ExperimentalCmd, userSettingTest));
Expand All @@ -67,7 +67,7 @@ TEST_CASE("ExperimentalFeature ExperimentalCmd", "[experimentalFeature]")
GroupPolicyTestOverride policies{ policiesKey.get() };

std::string_view json = R"({ "experimentalFeatures": { "experimentalCmd": true } })";
SetSetting(Streams::PrimaryUserSettings, json);
SetSetting(Stream::PrimaryUserSettings, json);
UserSettingsTest userSettingTest;

REQUIRE_FALSE(ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::ExperimentalCmd, userSettingTest));
Expand Down
5 changes: 3 additions & 2 deletions src/AppInstallerCLITests/PreIndexedPackageSource.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.
#include "pch.h"
#include "TestCommon.h"
#include "TestSettings.h"
#include <AppInstallerRepositorySource.h>
#include <AppInstallerRuntime.h>
#include <AppInstallerStrings.h>
Expand Down Expand Up @@ -56,8 +57,8 @@ std::string GetContents(const fs::path& file)

void CleanSources()
{
RemoveSetting(Streams::UserSources);
RemoveSetting(Streams::SourcesMetadata);
RemoveSetting(Stream::UserSources);
RemoveSetting(Stream::SourcesMetadata);
fs::remove_all(GetPathToFileDir());
}

Expand Down
160 changes: 132 additions & 28 deletions src/AppInstallerCLITests/Settings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ TEST_CASE("ReadEmptySetting", "[settings]")
{
StreamDefinition name{ Type::Standard, "nonexistentsetting" };

auto result = GetSettingStream(name);
auto result = Stream{ name }.Get();
REQUIRE(!result);
}

Expand All @@ -24,9 +24,10 @@ TEST_CASE("SetAndReadSetting", "[settings]")
StreamDefinition name{ Type::Standard, "testsettingname" };
std::string value = "This is the test setting value";

SetSetting(name, value);
Stream stream{ name };
REQUIRE(stream.Set(value));

auto result = GetSettingStream(name);
auto result = stream.Get();
REQUIRE(static_cast<bool>(result));

std::string settingValue = ReadEntireStream(*result);
Expand All @@ -38,9 +39,10 @@ TEST_CASE("SetAndReadSettingInContainer", "[settings]")
StreamDefinition name{ Type::Standard, "testcontainer/testsettingname" };
std::string value = "This is the test setting value from inside a container";

SetSetting(name, value);
Stream stream{ name };
REQUIRE(stream.Set(value));

auto result = GetSettingStream(name);
auto result = stream.Get();
REQUIRE(static_cast<bool>(result));

std::string settingValue = ReadEntireStream(*result);
Expand All @@ -52,19 +54,20 @@ TEST_CASE("RemoveSetting", "[settings]")
StreamDefinition name{ Type::Standard, "testsettingname" };
std::string value = "This is the test setting value to be removed";

SetSetting(name, value);
Stream stream{ name };
REQUIRE(stream.Set(value));

{
auto result = GetSettingStream(name);
auto result = stream.Get();
REQUIRE(static_cast<bool>(result));

std::string settingValue = ReadEntireStream(*result);
REQUIRE(value == settingValue);
}

RemoveSetting( name);
stream.Remove();

auto result = GetSettingStream(name);
auto result = stream.Get();
REQUIRE(!static_cast<bool>(result));
}

Expand All @@ -73,9 +76,10 @@ TEST_CASE("SetAndReadUserFileSetting", "[settings]")
StreamDefinition name{ Type::UserFile, "userfilesetting" };
std::string value = "This is the test setting value for a user file";

SetSetting(name, value);
Stream stream{ name };
REQUIRE(stream.Set(value));

auto result = GetSettingStream(name);
auto result = stream.Get();
REQUIRE(static_cast<bool>(result));

std::string settingValue = ReadEntireStream(*result);
Expand All @@ -86,7 +90,7 @@ TEST_CASE("ReadEmptySecureSetting", "[settings]")
{
StreamDefinition name{ Type::Secure, "secure_nonexistentsetting" };

auto result = GetSettingStream(name);
auto result = Stream{ name }.Get();
REQUIRE(!result);
}

Expand All @@ -95,9 +99,10 @@ TEST_CASE("SetAndReadSecureSetting", "[settings]")
StreamDefinition name{ Type::Secure, "secure_testsettingname" };
std::string value = "This is the test setting value";

SetSetting(name, value);
Stream stream{ name };
REQUIRE(stream.Set(value));

auto result = GetSettingStream(name);
auto result = stream.Get();
REQUIRE(static_cast<bool>(result));

std::string settingValue = ReadEntireStream(*result);
Expand All @@ -109,9 +114,10 @@ TEST_CASE("SetAndReadSecureSettingInContainer", "[settings]")
StreamDefinition name{ Type::Secure, "testcontainer/secure_testsettingname" };
std::string value = "This is the test setting value from inside a container";

SetSetting(name, value);
Stream stream{ name };
REQUIRE(stream.Set(value));

auto result = GetSettingStream(name);
auto result = stream.Get();
REQUIRE(static_cast<bool>(result));

std::string settingValue = ReadEntireStream(*result);
Expand All @@ -123,19 +129,20 @@ TEST_CASE("RemoveSecureSetting", "[settings]")
StreamDefinition name{ Type::Secure, "secure_testsettingname" };
std::string value = "This is the test setting value to be removed";

SetSetting(name, value);
Stream stream{ name };
REQUIRE(stream.Set(value));

{
auto result = GetSettingStream(name);
auto result = stream.Get();
REQUIRE(static_cast<bool>(result));

std::string settingValue = ReadEntireStream(*result);
REQUIRE(value == settingValue);
}

RemoveSetting(name);
stream.Remove();

auto result = GetSettingStream(name);
auto result = stream.Get();
REQUIRE(!static_cast<bool>(result));
}

Expand All @@ -144,27 +151,29 @@ TEST_CASE("SetAndReadSecureSetting_SecureDataRemoved", "[settings]")
StreamDefinition name{ Type::Secure, "secure_testsettingname" };
std::string value = "This is the test setting value";

SetSetting(name, value);
Stream stream{ name };
REQUIRE(stream.Set(value));

auto result = GetSettingStream(name);
auto result = stream.Get();
REQUIRE(static_cast<bool>(result));

std::string settingValue = ReadEntireStream(*result);
REQUIRE(value == settingValue);

std::filesystem::remove(GetPathTo(PathName::SecureSettings) / name.Path);
std::filesystem::remove(GetPathTo(PathName::SecureSettings) / name.Name);

REQUIRE_THROWS_HR(GetSettingStream(name), SPAPI_E_FILE_HASH_NOT_IN_CATALOG);
REQUIRE_THROWS_HR(stream.Get(), SPAPI_E_FILE_HASH_NOT_IN_CATALOG);
}

TEST_CASE("SetAndReadSecureSetting_DataTampered", "[settings]")
{
StreamDefinition name{ Type::Secure, "secure_testsettingname" };
std::string value = "This is the test setting value";

SetSetting(name, value);
Stream stream{ name };
REQUIRE(stream.Set(value));

auto result = GetSettingStream(name);
auto result = stream.Get();
REQUIRE(static_cast<bool>(result));

std::string settingValue = ReadEntireStream(*result);
Expand All @@ -173,7 +182,102 @@ TEST_CASE("SetAndReadSecureSetting_DataTampered", "[settings]")
StreamDefinition insecureName = name;
insecureName.Type = Type::Standard;

SetSetting(insecureName, "Tampered data");
REQUIRE(Stream{ insecureName }.Set("Tampered data"));

REQUIRE_THROWS_HR(GetSettingStream(name), HRESULT_FROM_WIN32(ERROR_DATA_CHECKSUM_ERROR));
REQUIRE_THROWS_HR(stream.Get(), HRESULT_FROM_WIN32(ERROR_DATA_CHECKSUM_ERROR));
}

TEST_CASE("SetChangeAndReadSetting", "[settings]")
{
StreamDefinition name{ Type::Standard, "testsettingname" };
std::string value1 = "This is the test setting value1";
std::string value2 = "This is the test setting value2, which is different";
std::string value3 = "This is the test setting value3; also different";

name.Type = GENERATE(Type::Standard, Type::Secure);
INFO(ToString(name.Type));

// Set the value on stream 1
Stream stream1{ name };
REQUIRE(stream1.Set(value1));

// Read the value on stream 2 to verify
{
Stream stream2{ name };

auto result = stream2.Get();
REQUIRE(static_cast<bool>(result));

std::string settingValue = ReadEntireStream(*result);
REQUIRE(value1 == settingValue);

// Set the value on stream 2
REQUIRE(stream2.Set(value2));
}

// Attempt to set the value on stream 1 again
REQUIRE(!stream1.Set(value3));

// Attempting to set again should still not work
REQUIRE(!stream1.Set(value3));

// Ensure that the value remains value 2
auto result = stream1.Get();
REQUIRE(static_cast<bool>(result));

std::string settingValue = ReadEntireStream(*result);
REQUIRE(value2 == settingValue);

// Now that we have read it, we can update it
REQUIRE(stream1.Set(value3));

result = stream1.Get();
REQUIRE(static_cast<bool>(result));

settingValue = ReadEntireStream(*result);
REQUIRE(value3 == settingValue);
}

TEST_CASE("AttemptSetOnNewValue", "[settings]")
{
StreamDefinition name{ Type::Standard, "testsettingname" };
std::string value1 = "This is the test setting value1";
std::string value2 = "This is the test setting value2, which is different";

name.Type = GENERATE(Type::Standard, Type::Secure);
INFO(ToString(name.Type));

// Remove the stream
Stream{ name }.Remove();

Stream stream1{ name };
REQUIRE(!stream1.Get());

// Set the value on stream 2
{
Stream stream2{ name };
REQUIRE(stream2.Set(value1));
}

// Attempt to set the value on stream 1 again
REQUIRE(!stream1.Set(value2));

// Attempting to set again should still not work
REQUIRE(!stream1.Set(value2));

// Ensure that the value remains value 2
auto result = stream1.Get();
REQUIRE(static_cast<bool>(result));

std::string settingValue = ReadEntireStream(*result);
REQUIRE(value1 == settingValue);

// Now that we have read it, we can update it
REQUIRE(stream1.Set(value2));

result = stream1.Get();
REQUIRE(static_cast<bool>(result));

settingValue = ReadEntireStream(*result);
REQUIRE(value2 == settingValue);
}
Loading

0 comments on commit fe8c617

Please sign in to comment.