CQ: handle more error conditions in try_job_on_rietveld.py

commit-queue/verification/try_job_on_rietveld.py

 # coding=utf8
 # Copyright (c) 2012 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.
 """Sends patches to the Try server and reads back results.
 
 - RietveldTryJobs contains RietveldTryJob, one per try job on a builder.
 - TryRunnerRietveld uses Rietveld to signal and poll job results.
 """
 
 import collections
+import errno
 import logging
 import re
+import socket
 import time
 import urllib2
 
 import buildbot_json
 import model
 from verification import base
 from verification import try_job_steps
 
 # A build running for longer than this is considered to be timed out.
 TIMED_OUT = 12 * 60 * 60
@@ skipping 742 matching lines @@
     self.send_status(pending, info)
 
   def _update_jobs_from_rietveld(self, pending, jobs, handle, now):
     """Grabs data from Rietveld and pass it to
     RietveldTryJobs.update_jobs_from_rietveld().
 
     Returns True on success.
     """
     status = buildbot_json.Buildbot(self.try_server_url)
     try:
-      data = self.context.rietveld.get_patchset_properties(
-          pending.issue, pending.patchset)
+      try:
+        data = self.context.rietveld.get_patchset_properties(
+            pending.issue, pending.patchset)
+      except urllib2.HTTPError as e:
+        if e.code == 404:
+          # TODO(phajdan.jr): Maybe generate a random id to correlate the user's
+          # error message and exception in the logs.
+          # Don't put exception traceback in the user-visible message to avoid
+          # leaking sensitive CQ data (passwords etc).
+          jobs.error_message = ('Failed to get patchset properties (patchset '
+                                'not found?)')
+          logging.error(str(e))
+          return False
+        else:

[Isaac (away), 2013/10/24 17:25:41]

Would this make more sense for rietveld.py to handle and return None?

[Paweł Hajdan Jr., 2013/10/24 18:28:12]

I don't think so. If anything, it might be better to raise a Rietveld-specific
exception wrapping urllib's one, but I don't think it's worth it here.

Returning None from Rietveld loses information about what exactly happened, and
this info can be useful to the client (as you see in this CL we do inspect the
exception and react a bit differently depending on error).

[Isaac (away), 2013/10/24 19:01:33]

Python 3 supports native exception chaining.  In Python 2, you can do this:

except urllib2.HTTPError as e:
    raise RietveldError(str(e)), None, sys.exc_info()[2]

Might be nice to have this logic into rietveld.py so that assumptions about
server behavior are in a single place.

+          raise
+
+      # Update the RietvedTryJobs object.
+      keys = jobs.update_jobs_from_rietveld(
+          pending.owner,
+          pending.issue,
+          data,
+          status,
+          self.context.checkout,
+          now)
     except urllib2.HTTPError as e:
-      if e.code == 404:
-        # TODO(phajdan.jr): Maybe generate a random id to correlate the user's
-        # error message and exception in the logs.
-        # Don't put exception traceback in the user-visible message to avoid
-        # leaking sensitive CQ data (passwords etc).
-        jobs.error_message = ('Failed to get patchset properties (patchset '
-                              'not found?)')
-        logging.error(str(e))
-        return False
-      elif e.code == 500:
+      if e.code in (500, 502, 503):
         # Temporary AppEngine hiccup. Just log it and return failure.
         logging.warning(str(e))
         return False
       else:
         raise
-    # Update the RietvedTryJobs object.
-    keys = jobs.update_jobs_from_rietveld(
-        pending.owner, pending.issue, data, status, self.context.checkout, now)
+    except socket.error as e:
+      # Temporary AppEngine hiccup. Just log it and return failure.
+      if e.errno == errno.ECONNRESET:

[Isaac (away), 2013/10/24 17:25:41]

We're now raising if this doesn't match?

[Paweł Hajdan Jr., 2013/10/24 18:28:12]

If we're catching it here it means something in the try block throws it, which
means previously it would just keep unwinding the stack.

Now we catch and handle just the connection reset, while other errors still
raise, just as before.

+        logging.warning(str(e))
+        return False
+      else:
+        raise
     if handle:
       for updated_key in keys:
         job = jobs.try_jobs[updated_key]
         self._update_dashboard(pending, job, status)
         jobs.signal_as_failed_if_needed(job, self._build_status_url(job), now)
 
     return True