[Predator] Add local cache for get command output.

appengine/findit/util_scripts/local_cache.py

+# Copyright 2015 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 logging
+import os
+import pickle
+import threading
+import zlib
+
+from lib.cache_decorator import Cacher
+
+
+class LocalCacher(Cacher):
+  """Cacher that uses local files to cache data."""
+  lock = threading.Lock()
+
+  def __init__(self, cache_dir):
+    self.cache_dir = cache_dir
+    if not os.path.exists(cache_dir): # pragma: no cover.

[wrengr, 2016/11/11 18:26:26]

Again, this sort of code has an inherent race condition. Should instead do
``try: os.makedirs(cache_dir); except OSException as e: if ...e is directory
already exists...: pass; else: ...handle actual problems...``

[Sharu Jiang, 2016/11/12 01:01:50]

How about using locks? Or there would be a lot exceptions created and thrown
out.

[wrengr, 2016/11/14 21:48:45]

The problem is in (the interaction with) the filesystem, not in Python itself.
Using a Python lock won't help because some other process could be the one to
create/remove the file/directory. No matter what programming language you're
using, by the time the kernel returns with the answer to ``os.path.exists`` that
answer is already stale wrt the filesystem itself. A lock within Python would
only protect against other threads in the same python process. A lockfile in the
filesystem is guaranteed to be shared across all processes, but that's just
pushing the problem elsewhere (since you need to make sure to access the
lockfile without causing the same race conditions).

There's nothing wrong with using ``try:except:`, it's the right tool for the job
here, and it's not expensive (especially when compared to the syscalls). The
only exceptions we want to discard are the ones where we tried to
``os.makedirs`` but the directory already exists; this is analogous to the
implicit ``else: pass`` branch in the current code. All other exceptions should
be passed along (just as the current code would raise those same exceptions).

+      os.makedirs(cache_dir)
+
+  def Get(self, key):
+    with LocalCacher.lock:
+      path = os.path.join(self.cache_dir, key)
+      if not os.path.exists(path):
+        return None

[wrengr, 2016/11/11 18:26:26]

You probably want this case to also do the logging before returning None, yes?
If so, then you can drop this whole conditional since the ``except`` block will
handle things.

[Sharu Jiang, 2016/11/12 01:01:50]

Done.

[Sharu Jiang, 2016/11/12 02:00:48]

I added back this, for local cacher won't cache for those None result, which
means for a lot of cases the path cannot be found and this would be expected,
thus on need to log it.

+
+      try:
+        with open(path) as f:
+          return pickle.loads(zlib.decompress(f.read()))
+      except Exception as e:  # pragma: no cover.
+        logging.exception('Failed loading cache: %s', e)
+        return None
+
+  def Set(self, key, data, expire_time=0):  # pylint: disable=W
+    with LocalCacher.lock:
+      try:
+        with open(os.path.join(self.cache_dir, key), 'wb') as f:
+          f.write(zlib.compress(pickle.dumps(data)))
+      except Exception as e:  # pragma: no cover.
+        logging.exception('Failed setting cache for key %s: %s', key, e)