Skip to content

Commit fe8c617

Browse files
JohnMcPMSJohnMcPMS
authored
Settings streams exchange semantics (microsoft#1534)
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.
1 parent 5a97e26 commit fe8c617

23 files changed

Lines changed: 1807 additions & 1083 deletions

‎src/AppInstallerCLITests/CustomHeader.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ TEST_CASE("RestClient_CustomHeader", "[RestSource][CustomHeader]")
8787

8888
TEST_CASE("AddSource_CustomHeader", "[RestSource][CustomHeader]")
8989
{
90-
SetSetting(Streams::UserSources, s_EmptySources);
90+
SetSetting(Stream::UserSources, s_EmptySources);
9191
TestHook_ClearSourceFactoryOverrides();
9292

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

112112
TEST_CASE("CreateSource_CustomHeader", "[RestSource][CustomHeader]")
113113
{
114-
SetSetting(Streams::UserSources, s_EmptySources);
114+
SetSetting(Stream::UserSources, s_EmptySources);
115115
TestHook_ClearSourceFactoryOverrides();
116116

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

139139
TEST_CASE("CreateSource_CustomHeaderNotApplicable", "[RestSource][CustomHeader]")
140140
{
141-
SetSetting(Streams::UserSources, s_EmptySources);
141+
SetSetting(Stream::UserSources, s_EmptySources);
142142
TestHook_ClearSourceFactoryOverrides();
143143

144144
std::string customHeader = "Testing custom header with open source";

‎src/AppInstallerCLITests/ExperimentalFeature.cpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,23 +39,23 @@ TEST_CASE("ExperimentalFeature ExperimentalCmd", "[experimentalFeature]")
3939
SECTION("Feature on")
4040
{
4141
std::string_view json = R"({ "experimentalFeatures": { "experimentalCmd": true } })";
42-
SetSetting(Streams::PrimaryUserSettings, json);
42+
SetSetting(Stream::PrimaryUserSettings, json);
4343
UserSettingsTest userSettingTest;
4444

4545
REQUIRE(ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::ExperimentalCmd, userSettingTest));
4646
}
4747
SECTION("Feature off")
4848
{
4949
std::string_view json = R"({ "experimentalFeatures": { "experimentalCmd": false } })";
50-
SetSetting(Streams::PrimaryUserSettings, json);
50+
SetSetting(Stream::PrimaryUserSettings, json);
5151
UserSettingsTest userSettingTest;
5252

5353
REQUIRE_FALSE(ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::ExperimentalCmd, userSettingTest));
5454
}
5555
SECTION("Invalid value")
5656
{
5757
std::string_view json = R"({ "experimentalFeatures": { "experimentalCmd": "string" } })";
58-
SetSetting(Streams::PrimaryUserSettings, json);
58+
SetSetting(Stream::PrimaryUserSettings, json);
5959
UserSettingsTest userSettingTest;
6060

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

6969
std::string_view json = R"({ "experimentalFeatures": { "experimentalCmd": true } })";
70-
SetSetting(Streams::PrimaryUserSettings, json);
70+
SetSetting(Stream::PrimaryUserSettings, json);
7171
UserSettingsTest userSettingTest;
7272

7373
REQUIRE_FALSE(ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::ExperimentalCmd, userSettingTest));

‎src/AppInstallerCLITests/PreIndexedPackageSource.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Licensed under the MIT License.
33
#include "pch.h"
44
#include "TestCommon.h"
5+
#include "TestSettings.h"
56
#include <AppInstallerRepositorySource.h>
67
#include <AppInstallerRuntime.h>
78
#include <AppInstallerStrings.h>
@@ -56,8 +57,8 @@ std::string GetContents(const fs::path& file)
5657

5758
void CleanSources()
5859
{
59-
RemoveSetting(Streams::UserSources);
60-
RemoveSetting(Streams::SourcesMetadata);
60+
RemoveSetting(Stream::UserSources);
61+
RemoveSetting(Stream::SourcesMetadata);
6162
fs::remove_all(GetPathToFileDir());
6263
}
6364

‎src/AppInstallerCLITests/Settings.cpp‎

Lines changed: 132 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ TEST_CASE("ReadEmptySetting", "[settings]")
1515
{
1616
StreamDefinition name{ Type::Standard, "nonexistentsetting" };
1717

18-
auto result = GetSettingStream(name);
18+
auto result = Stream{ name }.Get();
1919
REQUIRE(!result);
2020
}
2121

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

27-
SetSetting(name, value);
27+
Stream stream{ name };
28+
REQUIRE(stream.Set(value));
2829

29-
auto result = GetSettingStream(name);
30+
auto result = stream.Get();
3031
REQUIRE(static_cast<bool>(result));
3132

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

41-
SetSetting(name, value);
42+
Stream stream{ name };
43+
REQUIRE(stream.Set(value));
4244

43-
auto result = GetSettingStream(name);
45+
auto result = stream.Get();
4446
REQUIRE(static_cast<bool>(result));
4547

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

55-
SetSetting(name, value);
57+
Stream stream{ name };
58+
REQUIRE(stream.Set(value));
5659

5760
{
58-
auto result = GetSettingStream(name);
61+
auto result = stream.Get();
5962
REQUIRE(static_cast<bool>(result));
6063

6164
std::string settingValue = ReadEntireStream(*result);
6265
REQUIRE(value == settingValue);
6366
}
6467

65-
RemoveSetting( name);
68+
stream.Remove();
6669

67-
auto result = GetSettingStream(name);
70+
auto result = stream.Get();
6871
REQUIRE(!static_cast<bool>(result));
6972
}
7073

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

76-
SetSetting(name, value);
79+
Stream stream{ name };
80+
REQUIRE(stream.Set(value));
7781

78-
auto result = GetSettingStream(name);
82+
auto result = stream.Get();
7983
REQUIRE(static_cast<bool>(result));
8084

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

89-
auto result = GetSettingStream(name);
93+
auto result = Stream{ name }.Get();
9094
REQUIRE(!result);
9195
}
9296

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

98-
SetSetting(name, value);
102+
Stream stream{ name };
103+
REQUIRE(stream.Set(value));
99104

100-
auto result = GetSettingStream(name);
105+
auto result = stream.Get();
101106
REQUIRE(static_cast<bool>(result));
102107

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

112-
SetSetting(name, value);
117+
Stream stream{ name };
118+
REQUIRE(stream.Set(value));
113119

114-
auto result = GetSettingStream(name);
120+
auto result = stream.Get();
115121
REQUIRE(static_cast<bool>(result));
116122

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

126-
SetSetting(name, value);
132+
Stream stream{ name };
133+
REQUIRE(stream.Set(value));
127134

128135
{
129-
auto result = GetSettingStream(name);
136+
auto result = stream.Get();
130137
REQUIRE(static_cast<bool>(result));
131138

132139
std::string settingValue = ReadEntireStream(*result);
133140
REQUIRE(value == settingValue);
134141
}
135142

136-
RemoveSetting(name);
143+
stream.Remove();
137144

138-
auto result = GetSettingStream(name);
145+
auto result = stream.Get();
139146
REQUIRE(!static_cast<bool>(result));
140147
}
141148

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

147-
SetSetting(name, value);
154+
Stream stream{ name };
155+
REQUIRE(stream.Set(value));
148156

149-
auto result = GetSettingStream(name);
157+
auto result = stream.Get();
150158
REQUIRE(static_cast<bool>(result));
151159

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

155-
std::filesystem::remove(GetPathTo(PathName::SecureSettings) / name.Path);
163+
std::filesystem::remove(GetPathTo(PathName::SecureSettings) / name.Name);
156164

157-
REQUIRE_THROWS_HR(GetSettingStream(name), SPAPI_E_FILE_HASH_NOT_IN_CATALOG);
165+
REQUIRE_THROWS_HR(stream.Get(), SPAPI_E_FILE_HASH_NOT_IN_CATALOG);
158166
}
159167

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

165-
SetSetting(name, value);
173+
Stream stream{ name };
174+
REQUIRE(stream.Set(value));
166175

167-
auto result = GetSettingStream(name);
176+
auto result = stream.Get();
168177
REQUIRE(static_cast<bool>(result));
169178

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

176-
SetSetting(insecureName, "Tampered data");
185+
REQUIRE(Stream{ insecureName }.Set("Tampered data"));
177186

178-
REQUIRE_THROWS_HR(GetSettingStream(name), HRESULT_FROM_WIN32(ERROR_DATA_CHECKSUM_ERROR));
187+
REQUIRE_THROWS_HR(stream.Get(), HRESULT_FROM_WIN32(ERROR_DATA_CHECKSUM_ERROR));
188+
}
189+
190+
TEST_CASE("SetChangeAndReadSetting", "[settings]")
191+
{
192+
StreamDefinition name{ Type::Standard, "testsettingname" };
193+
std::string value1 = "This is the test setting value1";
194+
std::string value2 = "This is the test setting value2, which is different";
195+
std::string value3 = "This is the test setting value3; also different";
196+
197+
name.Type = GENERATE(Type::Standard, Type::Secure);
198+
INFO(ToString(name.Type));
199+
200+
// Set the value on stream 1
201+
Stream stream1{ name };
202+
REQUIRE(stream1.Set(value1));
203+
204+
// Read the value on stream 2 to verify
205+
{
206+
Stream stream2{ name };
207+
208+
auto result = stream2.Get();
209+
REQUIRE(static_cast<bool>(result));
210+
211+
std::string settingValue = ReadEntireStream(*result);
212+
REQUIRE(value1 == settingValue);
213+
214+
// Set the value on stream 2
215+
REQUIRE(stream2.Set(value2));
216+
}
217+
218+
// Attempt to set the value on stream 1 again
219+
REQUIRE(!stream1.Set(value3));
220+
221+
// Attempting to set again should still not work
222+
REQUIRE(!stream1.Set(value3));
223+
224+
// Ensure that the value remains value 2
225+
auto result = stream1.Get();
226+
REQUIRE(static_cast<bool>(result));
227+
228+
std::string settingValue = ReadEntireStream(*result);
229+
REQUIRE(value2 == settingValue);
230+
231+
// Now that we have read it, we can update it
232+
REQUIRE(stream1.Set(value3));
233+
234+
result = stream1.Get();
235+
REQUIRE(static_cast<bool>(result));
236+
237+
settingValue = ReadEntireStream(*result);
238+
REQUIRE(value3 == settingValue);
239+
}
240+
241+
TEST_CASE("AttemptSetOnNewValue", "[settings]")
242+
{
243+
StreamDefinition name{ Type::Standard, "testsettingname" };
244+
std::string value1 = "This is the test setting value1";
245+
std::string value2 = "This is the test setting value2, which is different";
246+
247+
name.Type = GENERATE(Type::Standard, Type::Secure);
248+
INFO(ToString(name.Type));
249+
250+
// Remove the stream
251+
Stream{ name }.Remove();
252+
253+
Stream stream1{ name };
254+
REQUIRE(!stream1.Get());
255+
256+
// Set the value on stream 2
257+
{
258+
Stream stream2{ name };
259+
REQUIRE(stream2.Set(value1));
260+
}
261+
262+
// Attempt to set the value on stream 1 again
263+
REQUIRE(!stream1.Set(value2));
264+
265+
// Attempting to set again should still not work
266+
REQUIRE(!stream1.Set(value2));
267+
268+
// Ensure that the value remains value 2
269+
auto result = stream1.Get();
270+
REQUIRE(static_cast<bool>(result));
271+
272+
std::string settingValue = ReadEntireStream(*result);
273+
REQUIRE(value1 == settingValue);
274+
275+
// Now that we have read it, we can update it
276+
REQUIRE(stream1.Set(value2));
277+
278+
result = stream1.Get();
279+
REQUIRE(static_cast<bool>(result));
280+
281+
settingValue = ReadEntireStream(*result);
282+
REQUIRE(value2 == settingValue);
179283
}

0 commit comments

Comments
 (0)