Devserver: allow SubversionFileSystem to be pinned to a specific rev on construction

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

Index: chrome/common/extensions/docs/server2/cron_servlet.py
diff --git a/chrome/common/extensions/docs/server2/cron_servlet.py b/chrome/common/extensions/docs/server2/cron_servlet.py
index e76f07423e2865d738b86011246ca2ca13edfca2..7b3488153221f1be247fb27f9724dd9ceb7d604f 100644
--- a/chrome/common/extensions/docs/server2/cron_servlet.py
+++ b/chrome/common/extensions/docs/server2/cron_servlet.py
@@ -6,9 +6,12 @@ import logging
 import time
 import traceback
 
-from appengine_wrappers import DeadlineExceededError, IsDevServer, logservice
+from app_yaml_helper import AppYamlHelper
+from appengine_wrappers import (
+    GetAppVersion, DeadlineExceededError, IsDevServer, logservice)
 from branch_utility import BranchUtility
 from caching_file_system import CachingFileSystem
+from empty_dir_file_system import EmptyDirFileSystem
 from github_file_system import GithubFileSystem
 from object_store_creator import ObjectStoreCreator
 from render_servlet import RenderServlet
@@ -18,20 +21,6 @@ from subversion_file_system import SubversionFileSystem
 import svn_constants
 from third_party.json_schema_compiler.memoize import memoize
 
-def _CreateServerInstanceForChannel(channel, delegate):
-  object_store_creator = ObjectStoreCreator(channel, start_empty=True)
-  branch = (delegate.CreateBranchUtility(object_store_creator)
-      .GetBranchForChannel(channel))
-  host_file_system = CachingFileSystem(
-      delegate.CreateHostFileSystemForBranch(branch),
-      object_store_creator)
-  app_samples_file_system = delegate.CreateAppSamplesFileSystem(
-      object_store_creator)
-  return ServerInstance(channel,
-                        object_store_creator,
-                        host_file_system,
-                        app_samples_file_system)
-
 class _SingletonRenderServletDelegate(RenderServlet.Delegate):
   def __init__(self, server_instance):
     self._server_instance = server_instance
@@ -44,16 +33,17 @@ class CronServlet(Servlet):
   '''
   def __init__(self, request, delegate_for_test=None):
     Servlet.__init__(self, request)
+    self._channel = request.path.strip('/')
     self._delegate = delegate_for_test or CronServlet.Delegate()
 
   class Delegate(object):
-    '''Allow runtime dependencies to be overridden for testing.
+    '''CronServlet's runtime dependencies. Override for testing.
     '''
     def CreateBranchUtility(self, object_store_creator):
       return BranchUtility.Create(object_store_creator)
 
-    def CreateHostFileSystemForBranch(self, branch):
-      return SubversionFileSystem.Create(branch)
+    def CreateHostFileSystemForBranchAndRevision(self, branch, revision):
+      return SubversionFileSystem.Create(branch, revision=revision)
 
     def CreateAppSamplesFileSystem(self, object_store_creator):
       # TODO(kalman): CachingFileSystem wrapper for GithubFileSystem, but it's
@@ -61,6 +51,9 @@ class CronServlet(Servlet):
       return (EmptyDirFileSystem() if IsDevServer() else
               GithubFileSystem.Create(object_store_creator))
 
+    def GetAppVersion(self):
+      return GetAppVersion()
+
   def Get(self):
     # Crons often time out, and when they do *and* then eventually try to
     # flush logs they die. Turn off autoflush and manually do so at the end.
@@ -77,12 +70,12 @@ class CronServlet(Servlet):
     # the time these won't have changed since the last cron run, so it's a
     # little wasteful, but hopefully rendering is really fast (if it isn't we
     # have a problem).
-    channel = self._request.path.strip('/')
+    channel = self._channel
     logging.info('cron/%s: starting' % channel)
 
     # This is returned every time RenderServlet wants to create a new
     # ServerInstance.
-    server_instance = _CreateServerInstanceForChannel(channel, self._delegate)
+    server_instance = self._GetSafeServerInstance()
 
     def get_via_render_servlet(path):
       return RenderServlet(
@@ -123,18 +116,23 @@ class CronServlet(Servlet):
     success = True
     try:
       # Render all of the publicly accessible files.
-      for path, path_prefix in (
-          # Note: rendering the public templates will pull in all of the private
-          # templates.
-          (svn_constants.PUBLIC_TEMPLATE_PATH, ''),
-          # Note: rendering the public templates will have pulled in the .js
-          # and manifest.json files (for listing examples on the API reference
-          # pages), but there are still images, CSS, etc.
-          (svn_constants.STATIC_PATH, 'static/'),
-          (svn_constants.EXAMPLES_PATH, 'extensions/examples/')):
-          # Note: don't try to short circuit any of this stuff. We want to run
-          # the cron for all the directories regardless of intermediate
-          # failures.
+      cron_runs = [
+        # Note: rendering the public templates will pull in all of the private
+        # templates.
+        (svn_constants.PUBLIC_TEMPLATE_PATH, ''),
+        # Note: rendering the public templates will have pulled in the .js
+        # and manifest.json files (for listing examples on the API reference
+        # pages), but there are still images, CSS, etc.
+        (svn_constants.STATIC_PATH, 'static/'),
+      ]
+      if not IsDevServer():
+        cron_runs.append(
+            (svn_constants.EXAMPLES_PATH, 'extensions/examples/'))
+
+      # Note: don't try to short circuit any of this stuff. We want to run
+      # the cron for all the directories regardless of intermediate
+      # failures.
+      for path, path_prefix in cron_runs:
         success = run_cron_for_dir(path, path_prefix=path_prefix) and success
 
       # TODO(kalman): Generic way for classes to request cron access. The next
@@ -143,22 +141,23 @@ class CronServlet(Servlet):
 
       # Extension examples have zip files too. Well, so do apps, but the app
       # file system doesn't get the Offline treatment so they don't need cron.
-      manifest_json = '/manifest.json'
-      example_zips = [
-          '%s.zip' % filename[:-len(manifest_json)]
-          for filename in server_instance.content_cache.GetFromFileListing(
-              svn_constants.EXAMPLES_PATH)
-          if filename.endswith(manifest_json)]
-      logging.info('cron/%s: rendering %s example zips...' % (
-          channel, len(example_zips)))
-      start_time = time.time()
-      try:
-        success = success and all(
-            get_via_render_servlet('extensions/examples/%s' % z).status == 200
-            for z in example_zips)
-      finally:
-        logging.info('cron/%s: rendering %s example zips took %s seconds' % (
-            channel, len(example_zips), time.time() - start_time))
+      if not IsDevServer():
+        manifest_json = '/manifest.json'
+        example_zips = [
+            '%s.zip' % filename[:-len(manifest_json)]
+            for filename in server_instance.content_cache.GetFromFileListing(
+                svn_constants.EXAMPLES_PATH)
+            if filename.endswith(manifest_json)]
+        logging.info('cron/%s: rendering %s example zips...' % (
+            channel, len(example_zips)))
+        start_time = time.time()
+        try:
+          success = success and all(
+              get_via_render_servlet('extensions/examples/%s' % z).status == 200
+              for z in example_zips)
+        finally:
+          logging.info('cron/%s: rendering %s example zips took %s seconds' % (
+              channel, len(example_zips), time.time() - start_time))
 
       # Also trigger a redirect so that PathCanonicalizer has an opportunity to
       # cache file listings.
@@ -172,3 +171,61 @@ class CronServlet(Servlet):
 
     return (Response.Ok('Success') if success else
             Response.InternalError('Failure'))
+
+  def _GetSafeServerInstance(self):
+    '''Returns a ServerInstance with a host file system at a safe revision,
+    meaning the last revision that the current running version of the server
+    existed.
+    '''
+    channel = self._channel
+    delegate = self._delegate
+
+    server_instance_at_head = self._CreateServerInstance(channel, None)
+
+    get_branch_for_channel = self._GetBranchForChannel

[cduvall, 2013/05/10 06:45:19]

is this line necessary? does it not like you using self._GetBranchForChannel
directly below?

[not at google - send to devlin, 2013/05/10 17:49:58]

Yeah, I either do this or save a reference to CronServlet's self object, since
the self in this scope overrides it. I don't really know what the idiom is here.

+    class AppYamlHelperDelegate(AppYamlHelper.Delegate):
+      def GetHostFileSystemForRevision(self, revision):
+        return delegate.CreateHostFileSystemForBranchAndRevision(
+            get_branch_for_channel(channel),
+            revision)
+
+    app_yaml_handler = AppYamlHelper(
+        svn_constants.APP_YAML_PATH,
+        server_instance_at_head.host_file_system,
+        AppYamlHelperDelegate(),
+        server_instance_at_head.object_store_creator)
+
+    if app_yaml_handler.IsUpToDate(delegate.GetAppVersion()):
+      return server_instance_at_head

[方觉(Fang Jue), 2013/05/10 08:49:17]

I was wondering what will happen if at this point a new commit (with changes to
both server2 and templates) happens to come in. Perhaps it would better to first
determine current head revision, get app.yaml version, and then pin to that head
revision if the server is up-to-date?

[not at google - send to devlin, 2013/05/10 17:49:58]

Yes true. I have a bug filed about this. There's actually already a bug here
that this won't fix, namely that while the cron is updating these files any
instances in the middle of querying stuff (e.g. static resources) will be
temporarily incorrect. That's more insidious - it'll actually happen, whereas
the race condition you describe ... probably won't.

I had a go at fixing it but it's a yak shave to make the paths work properly
when just reading '/'. I'll do it in a follow-up, gotta submit this today.

+
+    # The version in app.yaml is greater than the currently running app's.
+    # The safe version is the one before it changed.
+    safe_revision = app_yaml_handler.GetFirstRevisionGreaterThan(
+        delegate.GetAppVersion()) - 1
+
+    logging.info('cron/%s: app version %s is out of date, safe is %s' % (
+        channel, delegate.GetAppVersion(), safe_revision))
+
+    return self._CreateServerInstance(channel, safe_revision)
+
+  def _CreateObjectStoreCreator(self, channel):
+    return ObjectStoreCreator(channel, start_empty=True)
+
+  def _GetBranchForChannel(self, channel):
+    object_store_creator = self._CreateObjectStoreCreator(channel)
+    return (self._delegate.CreateBranchUtility(object_store_creator)
+        .GetBranchForChannel(channel))
+
+  def _CreateServerInstance(self, channel, revision):
+    object_store_creator = self._CreateObjectStoreCreator(channel)
+    host_file_system = CachingFileSystem(
+        self._delegate.CreateHostFileSystemForBranchAndRevision(
+            self._GetBranchForChannel(channel),
+            revision),
+        object_store_creator)
+    app_samples_file_system = self._delegate.CreateAppSamplesFileSystem(
+        object_store_creator)
+    return ServerInstance(channel,
+                          object_store_creator,
+                          host_file_system,
+                          app_samples_file_system)