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

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

Index: chrome/common/extensions/docs/server2/caching_file_system.py
diff --git a/chrome/common/extensions/docs/server2/caching_file_system.py b/chrome/common/extensions/docs/server2/caching_file_system.py
index ecadb68ab5d4ee6938ff51e64da4e69c4ef1e37c..0bca4e815e699541887125062c97d9effa7f5173 100644
--- a/chrome/common/extensions/docs/server2/caching_file_system.py
+++ b/chrome/common/extensions/docs/server2/caching_file_system.py
@@ -57,7 +57,7 @@ class CachingFileSystem(FileSystem):
 
     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
@@ -107,7 +107,12 @@ class CachingFileSystem(FileSystem):
     # 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.
@@ -118,6 +123,11 @@ class CachingFileSystem(FileSystem):
       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.