Raise exceptions properly on HTTP errors from OAuthRpcServer (which is only used on bots)

rietveld.py

 # coding: utf-8
 # 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.
 """Defines class Rietveld to easily access a rietveld instance.
 
 Security implications:
 
 The following hypothesis are made:
 - Rietveld enforces:
   - Nobody else than issue owner can upload a patch set
   - Verifies the issue owner credentials when creating new issues
   - A issue owner can't change once the issue is created
   - A patch set cannot be modified
 """
 
 import copy
 import errno
 import json
 import logging
 import re
 import socket
 import ssl
+import StringIO
 import sys
 import time
 import urllib
 import urllib2
 import urlparse
 
 import patch
 
 from third_party import upload
 import third_party.oauth2client.client as oa2client
@@ skipping 368 matching lines @@
     try:
       # Sadly, upload.py calls ErrorExit() which does a sys.exit(1) on HTTP
       # 500 in AbstractRpcServer.Send().
       old_error_exit = upload.ErrorExit
       def trap_http_500(msg):
         """Converts an incorrect ErrorExit() call into a HTTPError exception."""
         m = re.search(r'(50\d) Server Error', msg)
         if m:
           # Fake an HTTPError exception. Cheezy. :(
           raise urllib2.HTTPError(
-              request_path, int(m.group(1)), msg, None, None)
+              request_path, int(m.group(1)), msg, None, StringIO.StringIO())
         old_error_exit(msg)
       upload.ErrorExit = trap_http_500
 
       for retry in xrange(self._maxtries):
         try:
           logging.debug('%s' % request_path)
-          result = self.rpc_server.Send(request_path, **kwargs)
-          # Sometimes GAE returns a HTTP 200 but with HTTP 500 as the content.
-          # How nice.
-          return result
+          return self.rpc_server.Send(request_path, **kwargs)
         except urllib2.HTTPError, e:
           if retry >= (self._maxtries - 1):
             raise
-          flake_codes = [500, 502, 503]
+          flake_codes = {500, 502, 503}
           if retry_on_404:
-            flake_codes.append(404)
+            flake_codes.add(404)
           if e.code not in flake_codes:
             raise
         except urllib2.URLError, e:
           if retry >= (self._maxtries - 1):
             raise
 
           def is_transient():
             # The idea here is to retry if the error isn't permanent.
             # Unfortunately, there are so many different possible errors,
             # that we end up enumerating those that are known to us to be
             # transient.
             # The reason can be a string or another exception, e.g.,
             # socket.error or whatever else.
             reason_as_str = str(e.reason)
-            for retry_anyway in [
+            for retry_anyway in (
                 'Name or service not known',
                 'EOF occurred in violation of protocol',
-                'timed out']:
+                'timed out'):
               if retry_anyway in reason_as_str:
                 return True
             return False  # Assume permanent otherwise.
           if not is_transient():
             raise
         except socket.error, e:
           if retry >= (self._maxtries - 1):
             raise
           if not 'timed out' in str(e):
             raise
@@ skipping 64 matching lines @@
            extra_headers=None,
            **kwargs):
     """Send a POST or GET request to the server.
 
     Args:
       request_path: path on the server to hit. This is concatenated with the
         value of 'host' provided to the constructor.
       payload: request is a POST if not None, GET otherwise
       timeout: in seconds
       extra_headers: (dict)
+
+    Returns: the HTTP response body as a string
+
+    Raises:
+      urllib2.HTTPError
     """
     # This method signature should match upload.py:AbstractRpcServer.Send()
     method = 'GET'
 
     headers = self.extra_headers.copy()
     headers.update(extra_headers or {})
 
     if payload is not None:
       method = 'POST'
       headers['Content-Type'] = content_type
 
     prev_timeout = self._http.timeout
     try:
       if timeout:
         self._http.timeout = timeout
-      # TODO(pgervais) implement some kind of retry mechanism (see upload.py).
       url = self.host + request_path
       if kwargs:
         url += "?" + urllib.urlencode(kwargs)
 
       # This weird loop is there to detect when the OAuth2 token has expired.
       # This is specific to appengine *and* rietveld. It relies on the
       # assumption that a 302 is triggered only by an expired OAuth2 token. This
       # prevents any usage of redirections in pages accessed this way.
 
       # This variable is used to make sure the following loop runs only twice.
       redirect_caught = False
       while True:
         try:
           ret = self._http.request(url,
                                    method=method,
                                    body=payload,
                                    headers=headers,
                                    redirections=0)
         except httplib2.RedirectLimit:
           if redirect_caught or method != 'GET':
             logging.error('Redirection detected after logging in. Giving up.')
             raise
           redirect_caught = True
           logging.debug('Redirection detected. Trying to log in again...')
           self.creds.access_token = None
           continue
         break
 
+      if ret[0].status >= 300:
+        raise urllib2.HTTPError(
+            request_path, int(ret[0]['status']), ret[1], None,
+            StringIO.StringIO())
+
       return ret[1]
 
     finally:
       self._http.timeout = prev_timeout
 
 
 class JwtOAuth2Rietveld(Rietveld):
   """Access to Rietveld using OAuth authentication.
 
   This class is supposed to be used only by bots, since this kind of
@@ skipping 152 matching lines @@
       self, issue, patchset, reason, clobber, revision, builders_and_tests,
       master=None, category='cq'):
     logging.info('ReadOnlyRietveld: triggering try jobs %r for issue %d' %
         (builders_and_tests, issue))
 
   def trigger_distributed_try_jobs(  # pylint:disable=R0201
       self, issue, patchset, reason, clobber, revision, masters,
       category='cq'):
     logging.info('ReadOnlyRietveld: triggering try jobs %r for issue %d' %
         (masters, issue))