Convert PpdCache and PpdProvider to TaskScheduler.

chromeos/printing/ppd_cache.cc

Index: chromeos/printing/ppd_cache.cc
diff --git a/chromeos/printing/ppd_cache.cc b/chromeos/printing/ppd_cache.cc
index 44abfd9565168c4122bf1cf794be57929bea3c7b..15cb85be27377e7ebc9873e625e186b8edce6d4c 100644
--- a/chromeos/printing/ppd_cache.cc
+++ b/chromeos/printing/ppd_cache.cc
@@ -15,6 +15,8 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/synchronization/lock.h"
+#include "base/task_runner_util.h"
+#include "base/task_scheduler/post_task.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
@@ -28,138 +30,121 @@ namespace chromeos {
 namespace printing {
 namespace {
 
-class PpdCacheImpl : public PpdCache {
- public:
-  PpdCacheImpl(const base::FilePath& cache_base_dir,
-               const scoped_refptr<base::SequencedTaskRunner>& disk_task_runner)
-      : cache_base_dir_(cache_base_dir), disk_task_runner_(disk_task_runner) {}
-
-  // Public API functions.
-  void Find(const std::string& key, const FindCallback& cb) override {
-    // Ensure the cache lives until the op is over.
-    AddRef();
-    ++inflight_ops_;
-    disk_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&PpdCacheImpl::FindImpl, this, key,
-                              base::SequencedTaskRunnerHandle::Get(), cb));
-  }
-
-  // Store the given contents at the given key.  If cb is non-null, it will
-  // be invoked on completion.
-  void Store(const std::string& key,
-             const std::string& contents,
-             const base::Callback<void()>& cb) override {
-    AddRef();
-    ++inflight_ops_;
-    disk_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&PpdCacheImpl::StoreImpl, this, key, contents,
-                              base::SequencedTaskRunnerHandle::Get(), cb));
-  }
-
-  bool Idle() const override { return inflight_ops_ == 0; }
-
- private:
-  ~PpdCacheImpl() override {}
+// Return the (full) path to the file we expect to find the given key at.
+base::FilePath FilePathForKey(const base::FilePath& base_dir,
+                              const std::string& key) {
+  std::string hashed_key = crypto::SHA256HashString(key);
+  return base_dir.Append(base::HexEncode(hashed_key.data(), hashed_key.size()));
+}
 
-  // If the cache doesn't already exist, create it.
-  void MaybeCreateCache() {
-    if (!base::PathExists(cache_base_dir_)) {
-      base::CreateDirectory(cache_base_dir_);
-    }
+// If the cache doesn't already exist, create it.
+void MaybeCreateCache(const base::FilePath& base_dir) {
+  if (!base::PathExists(base_dir)) {
+    base::CreateDirectory(base_dir);
   }
+}
 
-  // Find implementation, runs on the i/o thread.
-  void FindImpl(const std::string& key,
-                const scoped_refptr<base::SequencedTaskRunner>& callback_runner,
-                const FindCallback& cb) {
-    base::ThreadRestrictions::AssertIOAllowed();
-    FindResult result;
-    result.success = false;
-    MaybeCreateCache();
-    base::File file(FilePathForKey(key),
-                    base::File::FLAG_OPEN | base::File::FLAG_READ);
-
-    if (file.IsValid()) {
-      int64_t len = file.GetLength();
-      if (len >= static_cast<int64_t>(crypto::kSHA256Length) &&
-          len <= static_cast<int64_t>(kMaxPpdSizeBytes) +
-                     static_cast<int64_t>(crypto::kSHA256Length)) {
-        std::unique_ptr<char[]> buf(new char[len]);
-        if (file.ReadAtCurrentPos(buf.get(), len) == len) {
-          base::StringPiece contents(buf.get(), len - crypto::kSHA256Length);
-          base::StringPiece checksum(buf.get() + len - crypto::kSHA256Length,
-                                     crypto::kSHA256Length);
-          if (crypto::SHA256HashString(contents) == checksum) {
-            base::File::Info info;
-            if (file.GetInfo(&info)) {
-              result.success = true;
-              result.age = base::Time::Now() - info.last_modified;
-              contents.CopyToString(&result.contents);
-            }
-          } else {
-            LOG(ERROR) << "Bad checksum for cache key " << key;
+// Find implementation, blocks on file access.  Must be run on a thread that
+// allows I/O.
+PpdCache::FindResult FindImpl(const base::FilePath& cache_dir,
+                              const std::string& key) {
+  base::ThreadRestrictions::AssertIOAllowed();
+
+  PpdCache::FindResult result;
+  result.success = false;
+  MaybeCreateCache(cache_dir);

[Carlson, 2017/06/20 18:57:33]

This patch makes lookups higher priorities than stores, which is a good idea. 
Maybe we should remove cache creation on Find, and just do it in Store?  

If the cache directory doesn't exist, just return a miss here?

[skau, 2017/06/20 20:23:09]

That's a good idea!

+  base::File file(FilePathForKey(cache_dir, key),
+                  base::File::FLAG_OPEN | base::File::FLAG_READ);
+
+  if (file.IsValid()) {
+    int64_t len = file.GetLength();
+    if (len >= static_cast<int64_t>(crypto::kSHA256Length) &&
+        len <= static_cast<int64_t>(kMaxPpdSizeBytes) +
+                   static_cast<int64_t>(crypto::kSHA256Length)) {
+      std::unique_ptr<char[]> buf(new char[len]);
+      if (file.ReadAtCurrentPos(buf.get(), len) == len) {

[gab, 2017/06/20 20:36:07]

This allows concurrent read/writes to the same file (per using two sequences),
can't that result in IO errors?

+        base::StringPiece contents(buf.get(), len - crypto::kSHA256Length);
+        base::StringPiece checksum(buf.get() + len - crypto::kSHA256Length,
+                                   crypto::kSHA256Length);
+        if (crypto::SHA256HashString(contents) == checksum) {
+          base::File::Info info;
+          if (file.GetInfo(&info)) {
+            result.success = true;
+            result.age = base::Time::Now() - info.last_modified;
+            contents.CopyToString(&result.contents);
           }
+        } else {
+          LOG(ERROR) << "Bad checksum for cache key " << key;
         }
       }
     }
-    callback_runner->PostTask(FROM_HERE,
-                              base::Bind(&PpdCacheImpl::ReplyAndRelease, this,
-                                         base::Bind(cb, result)));
   }
 
-  // If |cb| is non-null, invoke it on this thread, then decrement the refcount.
-  void ReplyAndRelease(const base::Callback<void()>& cb) {
-    if (!cb.is_null()) {
-      cb.Run();
-    }
-    --inflight_ops_;
-    Release();
-    // Object may be destroyed here, so no further access to object data
-    // allowed.
-  }
+  return result;
+}
 
-  // Store implementation, runs on the i/o thread.
-  void StoreImpl(
-      const std::string& key,
-      const std::string& contents,
-      const scoped_refptr<base::SequencedTaskRunner>& callback_runner,
-      const base::Callback<void()> cb) {
-    base::ThreadRestrictions::AssertIOAllowed();
-    MaybeCreateCache();
-    if (contents.size() > kMaxPpdSizeBytes) {
-      LOG(ERROR) << "Ignoring attempt to cache large object";
-    } else {
-      auto path = FilePathForKey(key);
-      base::File file(path,
-                      base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
-      std::string checksum = crypto::SHA256HashString(contents);
-      if (!file.IsValid() ||
-          file.WriteAtCurrentPos(contents.data(), contents.size()) !=
-              static_cast<int>(contents.size()) ||
-          file.WriteAtCurrentPos(checksum.data(), checksum.size()) !=
-              static_cast<int>(checksum.size())) {
-        LOG(ERROR) << "Failed to create ppd cache file";
-        file.Close();
-        if (!base::DeleteFile(path, false)) {
-          LOG(ERROR) << "Failed to cleanup failed creation.";
-        }
+// Store implementation, blocks on file access.  Must be run on a thread that
+// allows I/O.
+void StoreImpl(const base::FilePath& cache_dir,
+               const std::string& key,
+               const std::string& contents) {
+  base::ThreadRestrictions::AssertIOAllowed();
+
+  MaybeCreateCache(cache_dir);
+  if (contents.size() > kMaxPpdSizeBytes) {
+    LOG(ERROR) << "Ignoring attempt to cache large object";
+  } else {
+    auto path = FilePathForKey(cache_dir, key);
+    base::File file(path,
+                    base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
+    std::string checksum = crypto::SHA256HashString(contents);
+    if (!file.IsValid() ||
+        file.WriteAtCurrentPos(contents.data(), contents.size()) !=
+            static_cast<int>(contents.size()) ||
+        file.WriteAtCurrentPos(checksum.data(), checksum.size()) !=
+            static_cast<int>(checksum.size())) {
+      LOG(ERROR) << "Failed to create ppd cache file";
+      file.Close();
+      if (!base::DeleteFile(path, false)) {
+        LOG(ERROR) << "Failed to cleanup failed creation.";
       }
     }
-    callback_runner->PostTask(
-        FROM_HERE, base::Bind(&PpdCacheImpl::ReplyAndRelease, this, cb));
+  }
+}
+
+class PpdCacheImpl : public PpdCache {
+ public:
+  explicit PpdCacheImpl(const base::FilePath& cache_base_dir)
+      : cache_base_dir_(cache_base_dir),
+        fetch_task_runner_(base::CreateSequencedTaskRunnerWithTraits(
+            {base::TaskPriority::USER_VISIBLE, base::MayBlock(),
+             base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})),
+        store_task_runner_(base::CreateSequencedTaskRunnerWithTraits(
+            {base::TaskPriority::BACKGROUND, base::MayBlock(),
+             base::TaskShutdownBehavior::BLOCK_SHUTDOWN})) {}
+
+  // Public API functions.
+  void Find(const std::string& key, const FindCallback& cb) override {
+    base::PostTaskAndReplyWithResult(
+        fetch_task_runner_.get(), FROM_HERE,
+        base::Bind(&FindImpl, cache_base_dir_, key), cb);
   }
 
-  // Return the (full) path to the file we expect to find the given key at.
-  base::FilePath FilePathForKey(const std::string& key) {
-    std::string hashed_key = crypto::SHA256HashString(key);
-    return cache_base_dir_.Append(
-        base::HexEncode(hashed_key.data(), hashed_key.size()));
+  // Store the given contents at the given key.  If cb is non-null, it will
+  // be invoked on completion.
+  void Store(const std::string& key,
+             const std::string& contents,
+             const base::Closure& cb) override {
+    store_task_runner_->PostTaskAndReply(
+        FROM_HERE, base::Bind(&StoreImpl, cache_base_dir_, key, contents), cb);
   }
 
-  int inflight_ops_ = 0;
+ private:
+  ~PpdCacheImpl() override {}
 
   base::FilePath cache_base_dir_;
-  scoped_refptr<base::SequencedTaskRunner> disk_task_runner_;
+  scoped_refptr<base::SequencedTaskRunner> fetch_task_runner_;
+  scoped_refptr<base::SequencedTaskRunner> store_task_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(PpdCacheImpl);
 };
@@ -167,11 +152,8 @@ class PpdCacheImpl : public PpdCache {
 }  // namespace
 
 // static
-scoped_refptr<PpdCache> PpdCache::Create(
-    const base::FilePath& cache_base_dir,
-    scoped_refptr<base::SequencedTaskRunner> disk_task_runner) {
-  return scoped_refptr<PpdCache>(
-      new PpdCacheImpl(cache_base_dir, disk_task_runner));
+scoped_refptr<PpdCache> PpdCache::Create(const base::FilePath& cache_base_dir) {
+  return scoped_refptr<PpdCache>(new PpdCacheImpl(cache_base_dir));
 }
 
 }  // namespace printing