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 88 matching lines @@
       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 = 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. """
     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(

[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.
     """
     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()
       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!')
@@ skipping 35 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()