Add blocking toggle to SetPersistentSettings. (#896) (#951)

Add blocking toggle to SetPersistentSettings to wait for CommitPendingWrite() to complete. Greatly improves reliability and accuracy of persistent_settings_test especially now that sleep can be removed. Added blocking toggle to RemovePersistentSettings as well. Replaces ASSERT with EXPECT when inside of closures that depend on test_done.Signal() to avoid deadlocking. b/280430510 b/283529011 (cherry picked from commit e024a33) Co-authored-by: Brian Ting <[email protected]>
youtube · Jul 17, 2023 · 3eeba6d · 3eeba6d
1 parent b1cd4de
commit 3eeba6d
Show file tree

Hide file tree

Showing 3 changed files with 116 additions and 103 deletions.
diff --git a/cobalt/persistent_storage/persistent_settings.cc b/cobalt/persistent_storage/persistent_settings.cc
@@ -201,20 +201,20 @@ PersistentSettings::GetPersistentSettingAsDictionary(const std::string& key) {
 
 void PersistentSettings::SetPersistentSetting(
     const std::string& key, std::unique_ptr<base::Value> value,
-    base::OnceClosure closure) {
+    base::OnceClosure closure, bool blocking) {
   if (key == kValidated) {
     LOG(ERROR) << "Cannot set protected persistent setting: " << key;
     return;
   }
   message_loop()->task_runner()->PostTask(
       FROM_HERE, base::BindOnce(&PersistentSettings::SetPersistentSettingHelper,
                                 base::Unretained(this), key, std::move(value),
-                                std::move(closure)));
+                                std::move(closure), blocking));
 }
 
 void PersistentSettings::SetPersistentSettingHelper(
     const std::string& key, std::unique_ptr<base::Value> value,
-    base::OnceClosure closure) {
+    base::OnceClosure closure, bool blocking) {
   DCHECK_EQ(base::MessageLoop::current(), message_loop());
   if (validated_initial_settings_) {
     base::AutoLock auto_lock(pref_store_lock_);
@@ -223,23 +223,25 @@ void PersistentSettings::SetPersistentSettingHelper(
         WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
     writeable_pref_store()->SetValue(
         key, std::move(value), WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
-    writeable_pref_store()->CommitPendingWrite();
+    commit_pending_write(blocking);
   } else {
     LOG(ERROR) << "Cannot set persistent setting while unvalidated: " << key;
   }
   std::move(closure).Run();
 }
 
 void PersistentSettings::RemovePersistentSetting(const std::string& key,
-                                                 base::OnceClosure closure) {
+                                                 base::OnceClosure closure,
+                                                 bool blocking) {
   message_loop()->task_runner()->PostTask(
       FROM_HERE,
       base::BindOnce(&PersistentSettings::RemovePersistentSettingHelper,
-                     base::Unretained(this), key, std::move(closure)));
+                     base::Unretained(this), key, std::move(closure),
+                     blocking));
 }
 
 void PersistentSettings::RemovePersistentSettingHelper(
-    const std::string& key, base::OnceClosure closure) {
+    const std::string& key, base::OnceClosure closure, bool blocking) {
   DCHECK_EQ(base::MessageLoop::current(), message_loop());
   if (validated_initial_settings_) {
     base::AutoLock auto_lock(pref_store_lock_);
@@ -248,7 +250,7 @@ void PersistentSettings::RemovePersistentSettingHelper(
         WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
     writeable_pref_store()->RemoveValue(
         key, WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS);
-    writeable_pref_store()->CommitPendingWrite();
+    commit_pending_write(blocking);
   } else {
     LOG(ERROR) << "Cannot remove persistent setting while unvalidated: " << key;
   }

diff --git a/cobalt/persistent_storage/persistent_settings.h b/cobalt/persistent_storage/persistent_settings.h
@@ -71,11 +71,13 @@ class PersistentSettings : public base::MessageLoop::DestructionObserver {
 
   void SetPersistentSetting(
       const std::string& key, std::unique_ptr<base::Value> value,
-      base::OnceClosure closure = std::move(base::DoNothing()));
+      base::OnceClosure closure = std::move(base::DoNothing()),
+      bool blocking = false);
 
   void RemovePersistentSetting(
       const std::string& key,
-      base::OnceClosure closure = std::move(base::DoNothing()));
+      base::OnceClosure closure = std::move(base::DoNothing()),
+      bool blocking = false);
 
   void DeletePersistentSettings(
       base::OnceClosure closure = std::move(base::DoNothing()));
@@ -89,10 +91,10 @@ class PersistentSettings : public base::MessageLoop::DestructionObserver {
 
   void SetPersistentSettingHelper(const std::string& key,
                                   std::unique_ptr<base::Value> value,
-                                  base::OnceClosure closure);
+                                  base::OnceClosure closure, bool blocking);
 
   void RemovePersistentSettingHelper(const std::string& key,
-                                     base::OnceClosure closure);
+                                     base::OnceClosure closure, bool blocking);
 
   void DeletePersistentSettingsHelper(base::OnceClosure closure);
 
@@ -101,6 +103,18 @@ class PersistentSettings : public base::MessageLoop::DestructionObserver {
     return pref_store_;
   }
 
+  void commit_pending_write(bool blocking) {
+    if (blocking) {
+      base::WaitableEvent written;
+      writeable_pref_store()->CommitPendingWrite(
+          base::OnceClosure(),
+          base::BindOnce(&base::WaitableEvent::Signal, Unretained(&written)));
+      written.Wait();
+    } else {
+      writeable_pref_store()->CommitPendingWrite();
+    }
+  }
+
   // Persistent settings file path.
   std::string persistent_settings_file_;