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/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) {