diff --git a/datastore.cpp b/datastore.cpp index d91ff34..40338c0 100644 --- a/datastore.cpp +++ b/datastore.cpp @@ -34,11 +34,11 @@ using namespace error; static string FilePath(const string& file_root, const string& suffix); static Result FileLoad(const string& file_path); -static Error FileStore(int transaction_depth, const string& file_path, const json& json); +static Error FileStore(const string& file_path, const json& json); Datastore::Datastore() : initialized_(false), explicit_lock_(mutex_, std::defer_lock), - transaction_depth_(0), json_(json::object()) { + transaction_depth_(0), transaction_dirty_(false), json_(json::object()) { } Error Datastore::Init(const string& file_root, const string& suffix) { @@ -58,7 +58,8 @@ Error Datastore::Init(const string& file_root, const string& suffix) { Error Datastore::Reset(const string& file_path, json new_value) { SYNCHRONIZE(mutex_); transaction_depth_ = 0; - if (auto err = FileStore(transaction_depth_, file_path, new_value)) { + transaction_dirty_ = false; + if (auto err = FileStore(file_path, new_value)) { return PassError(err); } json_ = new_value; @@ -80,6 +81,7 @@ void Datastore::BeginTransaction() { SYNCHRONIZE(mutex_); // We got a local lock, so we know there's no transaction in progress in any other thread. if (transaction_depth_ == 0) { + transaction_dirty_ = false; explicit_lock_.lock(); } transaction_depth_++; @@ -100,12 +102,18 @@ Error Datastore::EndTransaction(bool commit) { return nullerr; } - // We need to release the explicit lock on exit from ths function, no matter what. + // We need to release the explicit lock on exit from this function, no matter what. // We will "adopt" the lock into this lock_guard to ensure the unlock happens when it goes out of scope. std::lock_guard> lock_releaser(explicit_lock_, std::adopt_lock); + if (!transaction_dirty_) { + // No actual substantive changes were made during this transaction, so we will avoid + // writing to disk. Committing and rolling back are no-ops if there are no changes. + return nullerr; + } + if (commit) { - return PassError(FileStore(transaction_depth_, file_path_, json_)); + return PassError(FileStore(file_path_, json_)); } // We're rolling back -- revert to what's on disk @@ -126,8 +134,24 @@ error::Result Datastore::Get() const { Error Datastore::Set(const json::json_pointer& p, json v) { SYNCHRONIZE(mutex_); MUST_BE_INITIALIZED; + + // We will use the transaction mechanism to do the writing. It will also help prevent + // changes to the stored value between the time we check it and the time we set it. + BeginTransaction(); + + // Avoid modifying the datastore if the value is the same as what's already there. + bool changed = true; + try { + changed = (json_.at(p) != v); + } + catch (json::out_of_range&) { + // The key doesn't exist, so continue to set it. + } + json_[p] = v; - return PassError(FileStore(transaction_depth_, file_path_, json_)); + transaction_dirty_ = changed; + + return PassError(EndTransaction(true)); } static string FilePath(const string& file_root, const string& suffix) { @@ -171,7 +195,7 @@ static Result FileLoad(const string& file_path) { if (!utils::FileExists(file_path)) { // Check that we can write here by trying to store. auto empty_object = json::object(); - if (auto err = FileStore(false, file_path, empty_object)) { + if (auto err = FileStore(file_path, empty_object)) { return WrapError(err, "file doesn't exist and FileStore failed"); } @@ -203,11 +227,7 @@ static Result FileLoad(const string& file_path) { return json; } -static Error FileStore(int transaction_depth, const string& file_path, const json& json) { - if (transaction_depth > 0) { - return nullerr; - } - +static Error FileStore(const string& file_path, const json& json) { const auto temp_file_path = file_path + TEMP_EXT; const auto commit_file_path = file_path + COMMIT_EXT; diff --git a/datastore.hpp b/datastore.hpp index 2182833..5854856 100644 --- a/datastore.hpp +++ b/datastore.hpp @@ -66,8 +66,8 @@ class Datastore { /// EndTransaction is called. Transactions are re-enterable, but not nested. /// NOTE: Failing to call EndTransaction will result in undefined behaviour. void BeginTransaction(); - /// Ends an ongoing transaction writing. If commit is true, it writes the changes - /// immediately; if false it discards the changes. + /// Ends an ongoing transaction. If commit is true, it writes the changes immediately; + /// if false it discards the changes. /// Committing or rolling back inner transactions does nothing. Any errors during /// inner transactions that require the outermost transaction to be rolled back must /// be handled by the caller. @@ -121,6 +121,7 @@ class Datastore { mutable std::recursive_mutex mutex_; std::unique_lock explicit_lock_; int transaction_depth_; + bool transaction_dirty_; std::string file_path_; json json_; diff --git a/datastore_test.cpp b/datastore_test.cpp index 8aa3c3f..73fa820 100644 --- a/datastore_test.cpp +++ b/datastore_test.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include "gtest/gtest.h" #include "test_helpers.hpp" @@ -511,6 +512,100 @@ TEST_F(TestDatastore, SetTypes) ASSERT_EQ(*gotInt, wantInt); } +TEST_F(TestDatastore, SetWriteDedup) +{ + auto temp_dir = GetTempDir(); + auto ds_path = DatastoreFilepath(temp_dir, ds_suffix); + + Datastore ds; + auto err = ds.Init(temp_dir.c_str(), ds_suffix); + ASSERT_FALSE(err); + + auto k = "/k"_json_pointer; + err = ds.Set(k, "a"); + ASSERT_FALSE(err); + auto file_time1 = std::filesystem::last_write_time(ds_path); + + // Try setting the same value again + err = ds.Set(k, "a"); + ASSERT_FALSE(err); + auto file_time2 = std::filesystem::last_write_time(ds_path); + + // The file time should not have changed after setting the same value + ASSERT_EQ(file_time1, file_time2); + + // Change to a different value + err = ds.Set(k, "b"); + ASSERT_FALSE(err); + auto file_time3 = std::filesystem::last_write_time(ds_path); + + // The file should have been updated, so its time should be newer + ASSERT_GT(file_time3, file_time1); +} + +TEST_F(TestDatastore, TransactionWriteDedup) +{ + auto temp_dir = GetTempDir(); + auto ds_path = DatastoreFilepath(temp_dir, ds_suffix); + + Datastore ds; + auto err = ds.Init(temp_dir.c_str(), ds_suffix); + ASSERT_FALSE(err); + + auto k1 = "/k1"_json_pointer; + auto k2 = "/k2"_json_pointer; + err = ds.Set(k1, "a"); + ASSERT_FALSE(err); + err = ds.Set(k2, "a"); + ASSERT_FALSE(err); + auto file_time1 = std::filesystem::last_write_time(ds_path); + + ds.BeginTransaction(); + + // Try setting the same values again + err = ds.Set(k1, "a"); + ASSERT_FALSE(err); + err = ds.Set(k2, "a"); + ASSERT_FALSE(err); + + err = ds.EndTransaction(true); + ASSERT_FALSE(err); + auto file_time2 = std::filesystem::last_write_time(ds_path); + + // The file time should not have changed after setting the same values + ASSERT_EQ(file_time1, file_time2); + + ds.BeginTransaction(); + + // Change to a different value + err = ds.Set(k1, "b"); + ASSERT_FALSE(err); + err = ds.Set(k2, "b"); + ASSERT_FALSE(err); + + err = ds.EndTransaction(true); + ASSERT_FALSE(err); + auto file_time3 = std::filesystem::last_write_time(ds_path); + + // The file should have been updated, so its time should be newer + ASSERT_GT(file_time3, file_time1); + + ds.BeginTransaction(); + + // Changing and then rolling back should still work as expected + err = ds.Set(k1, "c"); + ASSERT_FALSE(err); + err = ds.Set(k2, "c"); + ASSERT_FALSE(err); + + err = ds.EndTransaction(false); + ASSERT_FALSE(err); + auto file_time4 = std::filesystem::last_write_time(ds_path); + + // Should not have changed + ASSERT_EQ(file_time3, file_time4); +} + TEST_F(TestDatastore, TypeMismatch) { Datastore ds; diff --git a/psicash.cpp b/psicash.cpp index a7b5f7a..3189d3e 100644 --- a/psicash.cpp +++ b/psicash.cpp @@ -161,9 +161,17 @@ void PsiCash::SetHTTPRequestFn(MakeHTTPRequestFn make_http_request_fn) { make_http_request_fn_ = std::move(make_http_request_fn); } -Error PsiCash::SetRequestMetadataItem(const string& key, const string& value) { +Error PsiCash::SetRequestMetadataItems(const std::map& items) { MUST_BE_INITIALIZED; - return PassError(user_data_->SetRequestMetadataItem(key, value)); + UserData::Transaction transaction(*user_data_); + for (const auto& it : items) { + // Errors won't manifest until we commit + (void)user_data_->SetRequestMetadataItem(it.first, it.second); + } + if (auto err = transaction.Commit()) { + return WrapError(err, "user data write failed"); + } + return nullerr; } Error PsiCash::SetLocale(const string& locale) { diff --git a/psicash.hpp b/psicash.hpp index 873cabc..1f4b700 100644 --- a/psicash.hpp +++ b/psicash.hpp @@ -194,7 +194,7 @@ class PsiCash { /// Set values that will be included in the request metadata. This includes /// client_version, client_region, sponsor_id, and propagation_channel_id. - error::Error SetRequestMetadataItem(const std::string& key, const std::string& value); + error::Error SetRequestMetadataItems(const std::map& items); /// Set current UI locale. error::Error SetLocale(const std::string& locale); diff --git a/psicash_test.cpp b/psicash_test.cpp index 5787be9..885943d 100644 --- a/psicash_test.cpp +++ b/psicash_test.cpp @@ -279,7 +279,7 @@ TEST_F(TestPsiCash, SetHTTPRequestFn) { } } -TEST_F(TestPsiCash, SetRequestMetadataItem) { +TEST_F(TestPsiCash, SetRequestMetadataItems) { PsiCashTester pc; auto err = pc.Init(TestPsiCash::UserAgent(), GetTempDir().c_str(), nullptr, false); ASSERT_FALSE(err); @@ -287,11 +287,19 @@ TEST_F(TestPsiCash, SetRequestMetadataItem) { auto j = pc.user_data().GetRequestMetadata(); ASSERT_EQ(j.size(), 0); - err = pc.SetRequestMetadataItem("k", "v"); + err = pc.SetRequestMetadataItems({{"k", "v"}}); ASSERT_FALSE(err); j = pc.user_data().GetRequestMetadata(); ASSERT_EQ(j["k"], "v"); + + err = pc.SetRequestMetadataItems({{"a", "b"}, {"x", "y"}}); + ASSERT_FALSE(err); + + j = pc.user_data().GetRequestMetadata(); + ASSERT_EQ(j["k"], "v"); + ASSERT_EQ(j["a"], "b"); + ASSERT_EQ(j["x"], "y"); } TEST_F(TestPsiCash, SetLocale) { @@ -934,7 +942,7 @@ TEST_F(TestPsiCash, ModifyLandingPage) { // With metadata // - err = pc.SetRequestMetadataItem("k", "v"); + err = pc.SetRequestMetadataItems({{"k", "v"}, {"x", "y"}}); ASSERT_FALSE(err); url_in = {"https://asdf.sadf.gf", "", ""}; res = pc.ModifyLandingPage(url_in.ToString()); @@ -942,7 +950,7 @@ TEST_F(TestPsiCash, ModifyLandingPage) { url_out.Parse(*res); ASSERT_EQ(url_out.scheme_host_path_, url_in.scheme_host_path_); ASSERT_EQ(url_out.fragment_, url_in.fragment_); - ASSERT_THAT(TokenPayloadsMatch(url_out.query_.substr(key_part.length()), R"({"metadata":{"k":"v"},"tokens":"kEarnerTokenType"})"_json), IsEmpty()); + ASSERT_THAT(TokenPayloadsMatch(url_out.query_.substr(key_part.length()), R"({"metadata":{"k":"v","x":"y"},"tokens":"kEarnerTokenType"})"_json), IsEmpty()); // // Errors @@ -1077,7 +1085,7 @@ TEST_F(TestPsiCash, GetRewardedActivityData) { ASSERT_TRUE(res); ASSERT_EQ(*res, base64::B64Encode(utils::Stringer(R"({"metadata":{"user_agent":")", TestPsiCash::UserAgent(), R"(","v":1},"tokens":"kEarnerTokenType","v":1})"))); - err = pc.SetRequestMetadataItem("k", "v"); + err = pc.SetRequestMetadataItems({{"k", "v"}}); ASSERT_FALSE(err); res = pc.GetRewardedActivityData(); ASSERT_TRUE(res); diff --git a/test_helpers.hpp b/test_helpers.hpp index 1fe495f..15f19c1 100644 --- a/test_helpers.hpp +++ b/test_helpers.hpp @@ -68,8 +68,16 @@ class TempDir return dev ? ".dev" : ".prod"; } + std::string DatastoreFilepath(const std::string& datastore_root, const char* suffix) { + return datastore_root + "/psicashdatastore" + suffix; + } + + std::string DatastoreFilepath(const std::string& datastore_root, const std::string& suffix) { + return DatastoreFilepath(datastore_root, suffix.c_str()); + } + std::string DatastoreFilepath(const std::string& datastore_root, bool dev) { - return datastore_root + "/psicashdatastore" + GetSuffix(dev); + return DatastoreFilepath(datastore_root, GetSuffix(dev)); } bool Write(const std::string& datastore_root, bool dev, const std::string& s) {