rebaseline_server: clean up thread locks

gm/rebaseline_server/server.py

 #!/usr/bin/python
 
 """
 Copyright 2013 Google Inc.
 
 Use of this source code is governed by a BSD-style license that can be
 found in the LICENSE file.
 
 HTTP server for our HTML rebaseline viewer.
 """
@@ skipping 50 matching lines @@
 
 DEFAULT_ACTUALS_DIR = '.gm-actuals'
 DEFAULT_EXPECTATIONS_DIR = os.path.join(TRUNK_DIRECTORY, 'expectations', 'gm')
 DEFAULT_PORT = 8888
 
 _HTTP_HEADER_CONTENT_LENGTH = 'Content-Length'
 _HTTP_HEADER_CONTENT_TYPE = 'Content-Type'
 
 _SERVER = None   # This gets filled in by main()
 
-def get_routable_ip_address():
+def _get_routable_ip_address():

[epoger, 2013/11/20 19:51:34]

made this function private (it's only used internally; let's not encourage
others to use it and thus be on the hook for maintaining it)

   """Returns routable IP address of this host (the IP address of its network
      interface that would be used for most traffic, not its localhost
      interface).  See http://stackoverflow.com/a/166589 """
   sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
   sock.connect(('8.8.8.8', 80))
   host = sock.getsockname()[0]
   sock.close()
   return host
 
+def _create_svn_checkout(dir_path, repo_url):
+  """Creates local checkout of an SVN repository at the specified directory
+  path, returning an svn.Svn object referring to the local checkout.
+
+  Args:
+    dir_path: path to the local checkout; if this directory does not yet exist,
+              it will be created and the repo will be checked out into it
+    repo_url: URL of SVN repo to check out into dir_path (unless the local
+              checkout already exists)
+  Returns: an svn.Svn object referring to the local checkout.
+  """
+  local_checkout = svn.Svn(dir_path)
+  if not os.path.isdir(dir_path):
+    os.makedirs(dir_path)
+    local_checkout.Checkout(repo_url, '.')
+  return local_checkout
+
 
 class Server(object):
   """ HTTP server for our HTML rebaseline viewer. """
 
   def __init__(self,
                actuals_dir=DEFAULT_ACTUALS_DIR,
                expectations_dir=DEFAULT_EXPECTATIONS_DIR,
                port=DEFAULT_PORT, export=False, editable=True,
                reload_seconds=0):
     """
     Args:
       actuals_dir: directory under which we will check out the latest actual
                    GM results
       expectations_dir: directory under which to find GM expectations (they
                         must already be in that directory)
       port: which TCP port to listen on for HTTP requests
       export: whether to allow HTTP clients on other hosts to access this server
       editable: whether HTTP clients are allowed to submit new baselines
       reload_seconds: polling interval with which to check for new results;
                       if 0, don't check for new results at all
     """
     self._actuals_dir = actuals_dir
     self._expectations_dir = expectations_dir
     self._port = port
     self._export = export
     self._editable = editable
     self._reload_seconds = reload_seconds
+    self._actuals_repo = _create_svn_checkout(
+        dir_path=actuals_dir, repo_url=ACTUALS_SVN_REPO)
+
+    # We only update the expectations dir if the server was run with a
+    # nonzero --reload argument; otherwise, we expect the user to maintain
+    # her own expectations as she sees fit.
+    #
+    # TODO(epoger): Use git instead of svn to check out expectations, since
+    # the Skia repo is moving to git.
+    if reload_seconds:
+      self._expectations_repo = _create_svn_checkout(
+          dir_path=expectations_dir, repo_url=EXPECTATIONS_SVN_REPO)
+    else:
+      self._expectations_repo = None
 
   def is_exported(self):
     """ Returns true iff HTTP clients on other hosts are allowed to access
     this server. """
     return self._export
 
   def is_editable(self):
     """ Returns true iff HTTP clients are allowed to submit new baselines. """
     return self._editable
 
   def reload_seconds(self):
     """ Returns the result reload period in seconds, or 0 if we don't reload
     results. """
     return self._reload_seconds
 
   def update_results(self):
     """ Create or update self.results, based on the expectations in
     self._expectations_dir and the latest actuals from skia-autogen.
     """
-    with self._svn_update_lock:
-      # self._svn_update_lock prevents us from updating the actual GM results
-      # in multiple threads simultaneously
-      logging.info('Updating actual GM results in %s from SVN repo %s ...' % (
-          self._actuals_dir, ACTUALS_SVN_REPO))
-      actuals_repo = svn.Svn(self._actuals_dir)
-      if not os.path.isdir(self._actuals_dir):
-        os.makedirs(self._actuals_dir)
-        actuals_repo.Checkout(ACTUALS_SVN_REPO, '.')
-      else:
-        actuals_repo.Update('.')
-      # We only update the expectations dir if the server was run with a
-      # nonzero --reload argument; otherwise, we expect the user to maintain
-      # her own expectations as she sees fit.
-      #
-      # self._svn_update_lock prevents us from updating the expected GM results
-      # in multiple threads simultaneously
-      #
-      # TODO(epoger): Use git instead of svn to check out expectations, since
-      # the Skia repo is moving to git.
-      if self._reload_seconds:
-        logging.info(
-            'Updating expected GM results in %s from SVN repo %s ...' % (
-            self._expectations_dir, EXPECTATIONS_SVN_REPO))
-        expectations_repo = svn.Svn(self._expectations_dir)
-        if not os.path.isdir(self._expectations_dir):
-          os.makedirs(self._expectations_dir)
-          expectations_repo.Checkout(EXPECTATIONS_SVN_REPO, '.')
-        else:
-          expectations_repo.Update('.')
-      # end of "with self._svn_update_lock:"
+    logging.info('Updating actual GM results in %s from SVN repo %s ...' % (
+        self._actuals_dir, ACTUALS_SVN_REPO))
+    self._actuals_repo.Update('.')
+
+    if self._expectations_repo:
+      logging.info(
+          'Updating expected GM results in %s from SVN repo %s ...' % (
+              self._expectations_dir, EXPECTATIONS_SVN_REPO))
+      self._expectations_repo.Update('.')
 
     logging.info(
           ('Parsing results from actuals in %s and expectations in %s, '
           + 'and generating pixel diffs (may take a while) ...') % (
           self._actuals_dir, self._expectations_dir))
-    new_results = results.Results(
+    self.results = results.Results(
         actuals_root=self._actuals_dir,
         expected_root=self._expectations_dir,
         generated_images_root=GENERATED_IMAGES_ROOT)
 
-    # Make sure we don't update self.results while a client is in the middle
-    # of reading from it.
-    with self.results_lock:
-      self.results = new_results
-
   def _result_reloader(self):
     """ If --reload argument was specified, reload results at the appropriate
     interval.
     """
     while self._reload_seconds:
       time.sleep(self._reload_seconds)
       self.update_results()
 
   def run(self):
-    self.results_lock = thread.allocate_lock()
-    self._svn_update_lock = thread.allocate_lock()
     self.update_results()
     thread.start_new_thread(self._result_reloader, ())
 
     if self._export:
       server_address = ('', self._port)
-      host = get_routable_ip_address()
+      host = _get_routable_ip_address()
       if self._editable:
         logging.warning('Running with combination of "export" and "editable" '
                         'flags.  Users on other machines will '
                         'be able to modify your GM expectations!')
     else:
       host = '127.0.0.1'
       server_address = (host, self._port)
     http_server = BaseHTTPServer.HTTPServer(server_address, HTTPRequestHandler)
     logging.info('Ready for requests on http://%s:%d' % (
         host, http_server.server_port))
@@ skipping 29 matching lines @@
 
   def do_GET_results(self, type):
     """ Handle a GET request for GM results.
 
     Args:
       type: string indicating which set of results to return;
             must be one of the results.RESULTS_* constants
     """
     logging.debug('do_GET_results: sending results of type "%s"' % type)
     try:
+      # Since we must make multiple calls to the Results object, grab a
+      # reference to it in case it is updated to point at a new Results
+      # object within another thread.
+      #
       # TODO(epoger): Rather than using a global variable for the handler
       # to refer to the Server object, make Server a subclass of
       # HTTPServer, and then it could be available to the handler via
       # the handler's .server instance variable.
+      results_obj = _SERVER.results
+      response_dict = results_obj.get_results_of_type(type)
+      time_updated = results_obj.get_timestamp()
 
-      with _SERVER.results_lock:
-        response_dict = _SERVER.results.get_results_of_type(type)
-        time_updated = _SERVER.results.get_timestamp()
       response_dict['header'] = {
         # Timestamps:
         # 1. when this data was last updated
         # 2. when the caller should check back for new data (if ever)
         #
         # We only return these timestamps if the --reload argument was passed;
         # otherwise, we have no idea when the expectations were last updated
         # (we allow the user to maintain her own expectations as she sees fit).
         'timeUpdated': time_updated if _SERVER.reload_seconds() else None,
         'timeNextUpdateAvailable': (
@@ skipping 89 matching lines @@
     if content_type != 'application/json;charset=UTF-8':
       raise Exception('unsupported %s [%s]' % (
           _HTTP_HEADER_CONTENT_TYPE, content_type))
 
     content_length = int(self.headers[_HTTP_HEADER_CONTENT_LENGTH])
     json_data = self.rfile.read(content_length)
     data = json.loads(json_data)
     logging.debug('do_POST_edits: received new GM expectations data [%s]' %
                   data)
 
-    with _SERVER.results_lock:
-      oldResultsType = data['oldResultsType']
-      oldResults = _SERVER.results.get_results_of_type(oldResultsType)
-      oldResultsHash = str(hash(repr(oldResults['testData'])))
-      if oldResultsHash != data['oldResultsHash']:
-        raise Exception('results of type "%s" changed while the client was '
-                        'making modifications. The client should reload the '
-                        'results and submit the modifications again.' %
-                        oldResultsType)
-      _SERVER.results.edit_expectations(data['modifications'])
+    # Since we must make multiple calls to the Results object, grab a
+    # reference to it in case it is updated to point at a new Results
+    # object within another thread.
+    results_obj = _SERVER.results
+    oldResultsType = data['oldResultsType']
+    oldResults = results_obj.get_results_of_type(oldResultsType)
+    oldResultsHash = str(hash(repr(oldResults['testData'])))
+    if oldResultsHash != data['oldResultsHash']:
+      raise Exception('results of type "%s" changed while the client was '
+                      'making modifications. The client should reload the '
+                      'results and submit the modifications again.' %
+                      oldResultsType)
+    results_obj.edit_expectations(data['modifications'])
 
     # Now that the edits have been committed, update results to reflect them.
     _SERVER.update_results()
 
   def redirect_to(self, url):
     """ Redirect the HTTP client to a different url.
 
     Args:
       url: URL to redirect the HTTP client to
     """
@@ skipping 73 matching lines @@
   args = parser.parse_args()
   global _SERVER
   _SERVER = Server(actuals_dir=args.actuals_dir,
                    expectations_dir=args.expectations_dir,
                    port=args.port, export=args.export, editable=args.editable,
                    reload_seconds=args.reload)
   _SERVER.run()
 
 if __name__ == '__main__':
   main()