CQ: handle more error conditions in try_job_on_rietveld.py

commit-queue/verification/try_job_on_rietveld.py

Index: commit-queue/verification/try_job_on_rietveld.py
===================================================================
--- commit-queue/verification/try_job_on_rietveld.py	(revision 229881)
+++ commit-queue/verification/try_job_on_rietveld.py	(working copy)
@@ -9,8 +9,10 @@
 """
 
 import collections
+import errno
 import logging
 import re
+import socket
 import time
 import urllib2
 
@@ -773,27 +775,44 @@
     """
     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]