Docserver: Add more skip_not_found support and cache "not found"s

chrome/common/extensions/docs/server2/caching_file_system.py

 # Copyright (c) 2012 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.
 
 import posixpath
 import sys
 
 from file_system import FileSystem, StatInfo, FileNotFoundError
 from future import Future
 from path_util import IsDirectory, ToDirectory
@@ skipping 39 matching lines @@
         return dir_stat
       # Was a file stat. Extract that file.
       file_version = dir_stat.child_versions.get(file_path)
       if file_version is None:
         raise FileNotFoundError('No stat found for %s in %s (found %s)' %
                                 (path, dir_path, dir_stat.child_versions))
       return StatInfo(file_version)
 
     dir_stat = self._stat_object_store.Get(dir_path).Get()
     if dir_stat is not None:
-      return Future(value=make_stat_info(dir_stat))
+      return Future(callback=lambda: make_stat_info(dir_stat))
 
     def next(dir_stat):
       assert dir_stat is not None  # should have raised a FileNotFoundError
       # We only ever need to cache the dir stat.
       self._stat_object_store.Set(dir_path, dir_stat)
       return make_stat_info(dir_stat)
     return self._MemoizedStatAsyncFromFileSystem(dir_path).Then(next)
 
   @memoize
   def _MemoizedStatAsyncFromFileSystem(self, dir_path):
     '''This is a simple wrapper to memoize Futures to directory stats, since
     StatAsync makes heavy use of it. Only cache directories so that the
     memoized cache doesn't blow up.
     '''
     assert IsDirectory(dir_path)
     return self._file_system.StatAsync(dir_path)
 
   def Read(self, paths, skip_not_found=False):
     '''Reads a list of files. If a file is in memcache and it is not out of

[not at google - send to devlin, 2014/08/26 20:59:34]

Could you change "memcache" to just "cached"?

     date, it is returned. Otherwise, the file is retrieved from the file system.
     '''
     cached_read_values = self._read_object_store.GetMulti(paths).Get()
     cached_stat_values = self._stat_object_store.GetMulti(paths).Get()
 
     # Populate a map of paths to Futures to their stat. They may have already
     # been cached in which case their Future will already have been constructed
     # with a value.
     stat_futures = {}
 
     def handle(error):
       if isinstance(error, FileNotFoundError):
         return None
       raise error
 
     for path in paths:
       stat_value = cached_stat_values.get(path)
       if stat_value is None:
         stat_future = self.StatAsync(path)
         if skip_not_found:
           stat_future = stat_future.Then(lambda x: x, handle)
       else:
         stat_future = Future(value=stat_value)
       stat_futures[path] = stat_future
 
     # Filter only the cached data which is fresh by comparing to the latest
     # stat. The cached read data includes the cached version. Remove it for
     # the result returned to callers.
     fresh_data = dict(

[not at google - send to devlin, 2014/08/26 20:59:34]

This "fresh" thing is tripping me up. Maybe it should be "uptodate_data".

         (path, data) for path, (data, version) in cached_read_values.iteritems()
-        if stat_futures[path].Get().version == version)
+        if version and stat_futures[path].Get().version == version)

[not at google - send to devlin, 2014/08/26 20:59:34]

I'm paranoid about version being 0 here. Could you do the "version is not None"
version of this check here?

Also mention in the comment *why* we're caching None as a value.

Which makes me wonder - do we need to do this? If we're already caching this
data in Stat, can you use that instead? It seems like that's exactly what you're
doing below.

+
+    if skip_not_found:
+      # Remove paths for files that don't exist so reads aren't attempted.

[not at google - send to devlin, 2014/08/26 20:59:34]

Could you mention why this works? Namely that this method turns any Stat
FileNotFoundErrors into None, so we can check its existence that way.

+      paths = [path for path in paths
+               if cached_read_values.get(path, (None, True))[1]]

[not at google - send to devlin, 2014/08/26 20:59:34]

Could you use 'path in cached_read_values'? This .get() form looks kinda
roundabout.

[ahernandez, 2014/08/27 00:07:22]

The reason I have the ugly .get() call is because I'm only filtering out paths
that are in cached_read_values *and* have a version == None. I can't use 'path
in cached_read_values' or a variation on that because that would filter out
paths that are valid but just haven't been cached.

[not at google - send to devlin, 2014/08/27 01:23:55]

Ah I see. So to be clear: If the item isn't cached yet then it'll return (None,
True), then we grab the True part and the path passes? Ok.

Could you add what you said as a comment?

 
     if len(fresh_data) == len(paths):
       # Everything was cached and up-to-date.
       return Future(value=fresh_data)
 
     def next(new_results):
       # Update the cache. This is a path -> (data, version) mapping.
       self._read_object_store.SetMulti(
           dict((path, (new_result, stat_futures[path].Get().version))
                for path, new_result in new_results.iteritems()))
+      # Update the read cache to include files that weren't found, to prevent
+      # constantly trying to read a file we now know doesn't exist.

[not at google - send to devlin, 2014/08/26 20:59:34]

Relating to comments above - I'm not sure we need to cache this, since the stat
cache already knows something similar. You could just update the results
directly, right?

[ahernandez, 2014/08/27 00:07:22]

I think the stat cache doesn't know about non-existent files, because the
stat_futures for those files are normally not called. What I can do is add a
loop that runs through all the futures and calls them, so that the stat_cache is
updated, and then I'm pretty sure I can get away with not having to
unnecessarily add to the read cache, if that's preferable. Then I shouldn't have
to check for the version being None in the fresh_data dict comp.

[not at google - send to devlin, 2014/08/27 01:23:55]

Ohh I see. Ok. What you have is fine.

We should also explain somewhere that the read cache holds files that we know
don't exist by using a None value for their cached stat value. Maybe when it's
declared?

And now I understand the "if version ..." check up on line 110. You should
comment (so many comments, ah well, this file is complicated) that the "version
is not None" check is again because we cache not-found files as a None version.

[ahernandez, 2014/08/27 17:33:28]

I don't think we should be afraid of comments :)

+      self._read_object_store.SetMulti(
+          dict((path, (None, None)) for path in paths
+               if stat_futures[path].Get() is None))
       new_results.update(fresh_data)
       return new_results
     # Read in the values that were uncached or old.
     return self._file_system.Read(set(paths) - set(fresh_data.iterkeys()),
                                   skip_not_found=skip_not_found).Then(next)
 
   def GetIdentity(self):
     return self._file_system.GetIdentity()
 
   def __repr__(self):
     return '%s of <%s>' % (type(self).__name__, repr(self._file_system))