rebaseline_server: improve thread locks to allow read access during updates

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 106 matching lines @@
 
   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.results_lock:
-      # self.results_lock prevents us from updating the actual GM results
+    with self._svn_update_lock:
+      # self._svn_update_lock prevents us from updating the actual GM results

[jcgregorio, 2013/11/08 17:33:21]

Comment seems wrong, doesn't this just prevent you from running multiple
update_results() at the same time?

       # 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.results_lock prevents us from updating the expected GM results
+      # 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(
+    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))
-      self.results = results.Results(
+    new_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()

[jcgregorio, 2013/11/08 17:33:21]

Add docs here for what each lock is used for.

+    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()
       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))
+    logging.info('Ready for requests on http://%s:%d' % (
+        host, http_server.server_port))
     http_server.serve_forever()
 
 
 class HTTPRequestHandler(BaseHTTPServer.BaseHTTPRequestHandler):
   """ HTTP request handlers for various types of queries this server knows
       how to handle (static HTML and Javascript, expected/actual results, etc.)
   """
   def do_GET(self):
     """ Handles all GET requests, forwarding them to the appropriate
         do_GET_* dispatcher. """
@@ skipping 239 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()