[pinpoint] Fix race condition in building + isolate finding.

dashboard/dashboard/pinpoint/models/quest/find_isolate.py

 # Copyright 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 from dashboard.pinpoint.models import isolate
 from dashboard.pinpoint.models.quest import execution
 from dashboard.pinpoint.models.quest import quest
 from dashboard.services import buildbucket_service
 
 
@@ skipping 31 matching lines @@
     self._target = target
     self._change = change
     self._build = None
 
   def _AsDict(self):
     return {
         'build': self._build,
     }
 
   def _Poll(self):
-    # Look for the .isolate in our cache.
+    if self._CheckIsolateCache():
+      return
+
+    if self._build:
+      self._CheckBuildStatus()
+      return
+
+    self._RequestBuild()
+
+  def _CheckIsolateCache(self):
+    """Checks the isolate cache to see if a build is already available.
+
+    Returns:
+      True iff the isolate was found and the execution is completed.

[perezju, 2017/09/13 13:46:38]

nit: Maybe this should be called "_CheckCompleted"? Which returns: "True if the
isolate was found, meaning the execution is completed."

[dtu, 2017/09/13 15:16:05]

Done.

+    """
     try:
       isolate_hash = isolate.Get(self._builder_name, self._change, self._target)
     except KeyError:
-      isolate_hash = None
+      return False
+    self._Complete(result_arguments={'isolate_hash': isolate_hash})
+    return True
 
-    if isolate_hash:
-      self._Complete(
-          result_arguments={'isolate_hash': isolate_hash})
+  def _CheckBuildStatus(self):
+    """Checks on the status of a previously requested build.
+
+    Raises:
+      BuildError: The build failed, was canceled, or didn't produce an isolate.
+    """
+    status = buildbucket_service.GetJobStatus(self._build)
+
+    if status['build']['status'] != 'COMPLETED':
       return
 
-    # Check the status of a previously requested build.
-    if self._build:
-      status = buildbucket_service.GetJobStatus(self._build)
+    if status['build']['result'] == 'FAILURE':
+      raise BuildError('Build failed: ' + status['build']['failure_reason'])
+    elif status['build']['result'] == 'CANCELED':
+      raise BuildError('Build was canceled: ' +
+                       status['build']['cancelation_reason'])
+    else:
+      if self._CheckIsolateCache():
+        return
+      raise BuildError('Buildbucket says the build completed successfully, '
+                       "but Pinpoint can't find the isolate hash.")
 
-      if status['build']['status'] != 'COMPLETED':
-        return
+  def _RequestBuild(self):
+    """Requests a build.
 
-      if status['build']['result'] == 'FAILURE':
-        raise BuildError('Build failed: ' + status['build']['failure_reason'])
-      elif status['build']['result'] == 'CANCELED':
-        raise BuildError('Build was canceled: ' +
-                         status['build']['cancelation_reason'])
-      else:
-        # It's possible for there to be a race condition if the builder uploads
-        # the isolate and completes the build between the above isolate lookup
-        # and buildbucket lookup, but right now, it takes builds a few minutes
-        # to package the build, so that doesn't happen.
-        raise BuildError('Buildbucket says the build completed successfully, '
-                         "but Pinpoint can't find the isolate hash.")
-
+    If a previous Execution already requested a build for this Change, returns
+    that build instead of requesting a new one.
+    """
     if self._change in self._previous_builds:
       # If another Execution already requested a build, reuse that one.
       self._build = self._previous_builds[self._change]
     else:
       # Request a build!
       buildbucket_info = _RequestBuild(self._builder_name, self._change)
       self._build = buildbucket_info['build']['id']
       self._previous_builds[self._change] = self._build
 
 
@@ skipping 43 matching lines @@
     # https://github.com/catapult-project/catapult/issues/3599
     parameters['properties'].update({
         'patch_storage': 'rietveld',
         'rietveld': change.patch.server,
         'issue': change.patch.issue,
         'patchset': change.patch.patchset,
     })
 
   # TODO: Look up Buildbucket bucket from builder_name.
   return buildbucket_service.Put(BUCKET, parameters)