Add sequencing comment to PpdCacheImpl.

chromeos/printing/ppd_cache.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/printing/ppd_cache.h"
 
 #include <utility>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 98 matching lines @@
             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.";
       }
     }
   }
 }
 
+// Implementation of the PpdCache that uses two separate task runners for Store
+// and Fetch since the two operations have different priorities.  It is okay
+// that Store and Fetch are not synchronized because we assume that we won't
+// overwrite a file that we're trying to read simultaneously.  If this
+// assumption is invalidated, the sequencing will require revisiting.

[Carlson, 2017/06/29 18:47:09]

The more I think about it, the more I'm convinced that we've gone a bit wrong
here.

I think we don't have any real guarantee here from sequencing.  We could have
concurrent stores and find for the same file going on within a single PpdCache
-- we could

Find(foo) - miss
Store(foo) - (kicks off actual disk io to store_task_runner_)
Find(foo) - (disk io on fetch_task_runner_) hit on file, but file is not
completely written yet, so get incomplete contents.

Furthermore, we could have *multiple* ppd providers instantiated at the same
time, which means we don't actually get any guarantees from things being in the
same object -- two objects could be doing entirely different things on the same
filesystem.

I think probably the checksum mechanism is good enough to make this all ok, but
I don't think this comment is right.

[skau, 2017/06/30 20:55:49]

Okay.  It's updated.  Really, what we're saying is that the sequences don't
matter, we actually need to synchronize with the file system.  I say a little
more about expected behavior instead of assumptions now.

 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})) {}
@@ skipping 26 matching lines @@
 
 }  // namespace
 
 // static
 scoped_refptr<PpdCache> PpdCache::Create(const base::FilePath& cache_base_dir) {
   return scoped_refptr<PpdCache>(new PpdCacheImpl(cache_base_dir));
 }
 
 }  // namespace printing
 }  // namespace chromeos