rebaseline_server: clean up thread locks

gm/rebaseline_server/server.py

Index: gm/rebaseline_server/server.py
===================================================================
--- gm/rebaseline_server/server.py	(revision 12295)
+++ gm/rebaseline_server/server.py	(working copy)
@@ -106,6 +106,24 @@
     self._editable = editable
     self._reload_seconds = reload_seconds
 
+    self._actuals_repo = svn.Svn(actuals_dir)

[epoger, 2013/11/14 20:21:56]

Now, the self._actuals_repo object is shared between threads, and is in charge
of synchronizing access to itself.  (Same for self._expectations_repo)

+    if not os.path.isdir(actuals_dir):
+      os.makedirs(actuals_dir)
+      self._actuals_repo.Checkout(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.
+    self._expectations_repo = None
+    if reload_seconds:
+      self._expectations_repo = svn.Svn(expectations_dir)
+      if not os.path.isdir(expectations_dir):

[jcgregorio, 2013/11/18 16:50:35]

Duplicated code from above, break out into a function.

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

Fixed in patchset 2.

+        os.makedirs(expectations_dir)
+        self._expectations_repo.Checkout(EXPECTATIONS_SVN_REPO, '.')
+
   def is_exported(self):
     """ Returns true iff HTTP clients on other hosts are allowed to access
     this server. """
@@ -124,52 +142,25 @@
     """ 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(

[epoger, 2013/11/14 20:21:56]

I *think* this is fine, because I assume self.results is not pointed away from
the old Results object and at the new Results object until the new one is
completely formed.

Any idea where I should look in Python's language specification for
documentation of that behavior?

[jcgregorio, 2013/11/18 16:50:35]

Yes, assignment won't happen until the RHS is evaluated completely.

On 2013/11/14 20:21:56, epoger wrote:

         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.
@@ -179,8 +170,6 @@
       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, ())
 
@@ -236,14 +225,18 @@
     """
     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
@@ -353,16 +346,19 @@
     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()