[Change Request] python-fedora two timeout fixes and a flask_fas_openid fix

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



We put an alpha version of python-fedora onto infrastructure just before
freeze.  This solved some very noisy issues for us but there are a few left.
I'd like to apply the attached hotfix which will fix three more problems:

* Increase the default timeout for http client requests from 30s to 120s.
  This should fix several scripts which are making queries to bodhi and
  pkgdb which take longer than 30s to complete.
* Catch timeout exceptions and reraise as ServerErrors.  This has two
  effects: The exception will be much more informative (the current code
  raises an error at an unrelated line because a value is not initialized).
  Code which already catches ServerErrors and proceeds will be able to
  proceed if the Server times out as well.
* fix flask_fas_openid where a variable being referenced was misnamed.
  We don't have any flask_fas_openid apps in production but this could be of
  benefit for people testing new apps in stg during freeze.

Could I get two +1s ?


-Toshio
diff -ur release/fedora/client/proxyclient.py p/fedora/client/proxyclient.py
--- release/fedora/client/proxyclient.py	2013-04-01 11:28:08.461340997 -0700
+++ p/fedora/client/proxyclient.py	2013-04-04 10:13:56.173838097 -0700
@@ -27,7 +27,8 @@
 import urllib
 import httplib
 import logging
-import requests
+# For handling an exception that's coming from requests:
+import ssl
 import time
 import warnings
 
@@ -46,6 +47,9 @@
 
 from bunch import bunchify
 from kitchen.text.converters import to_bytes
+import requests
+# For handling an exception that's coming from requests:
+import urllib3
 
 from fedora import __version__, b_
 from fedora.client import AppError, AuthError, ServerError
@@ -112,7 +116,7 @@
     .. attribute:: timeout
         A float describing the timeout of the connection. The timeout only
         affects the connection process itself, not the downloading of the
-        response body. Defaults to 30 seconds.
+        response body. Defaults to 120 seconds.
 
     .. versionchanged:: 0.3.33
         Added the timeout attribute
@@ -120,8 +124,8 @@
     log = log
 
     def __init__(self, base_url, useragent=None, session_name='tg-visit',
-            session_as_cookie=True, debug=False, insecure=False, retries=0,
-            timeout=30.0):
+            session_as_cookie=True, debug=False, insecure=False, retries=None,
+            timeout=None):
         '''Create a client configured for a particular service.
 
         :arg base_url: Base of every URL used to contact the server
@@ -144,7 +148,7 @@
             number makes it try forever.  Defaults to zero, no retries.
         :kwarg timeout: A float describing the timeout of the connection. The
             timeout only affects the connection process itself, not the downloading
-            of the response body. Defaults to 30 seconds.
+            of the response body. Defaults to 120 seconds.
 
         .. versionchanged:: 0.3.33
             Added the timeout kwarg
@@ -178,8 +182,17 @@
                 ' constructor with session_as_cookie=False'),
                 DeprecationWarning, stacklevel=2)
         self.insecure = insecure
-        self.retries = retries or 0
-        self.timeout = timeout or 30.0
+
+        # Have to do retries and timeout default values this way as BaseClient
+        # sends None for these values if not overridden by the user.
+        if retries is None:
+            retries = 0
+        else:
+            self.retries = retries
+        if timeout is None:
+            self.timeout = 120.0
+        else:
+            self.timeout = timeout
         self.log.debug(b_('proxyclient.__init__:exited'))
 
     def __get_debug(self):
@@ -381,18 +394,41 @@
                     verify=not self.insecure,
                     timeout=timeout,
                 )
-            except requests.Timeout:
+            except (requests.Timeout, requests.exceptions.SSLError) as e:
+                if isinstance(e, requests.exceptions.SSLError):
+                    # And now we know how not to code a library exception
+                    # hierarchy...  We're expecting that requests is raising
+                    # the following stupidity:
+                    # requests.exceptions.SSLError(
+                    #   urllib3.exceptions.SSLError(
+                    #     ssl.SSLError('The read operation timed out')))
+                    # If we weren't interested in reraising the exception with
+                    # full tracdeback we could use a try: except instead of
+                    # this gross conditional.  But we need to code defensively
+                    # because we don't want to raise an unrelated exception
+                    # here and if requests/urllib3 can do this sort of
+                    # nonsense, they may change the nonsense in the future
+                    if not (e.args and isinstance(e.args[0],
+                                                  urllib3.exceptions.SSLError)
+                            and e.args[0].args
+                            and isinstance(e.args[0].args[0], ssl.SSLError)
+                            and e.args[0].args[0].args
+                            and 'timed out' in e.args[0].args[0].args[0]):
+                        # We're only interested in timeouts here
+                        raise
                 self.log.debug(b_('Request timed out'))
                 if retries < 0 or num_tries < retries:
                     num_tries += 1
                     self.log.debug(b_('Attempt #%(try)s failed') % {'try': num_tries})
                     time.sleep(0.5)
                     continue
+                # Fail and raise an error
+                # Raising our own exception protects the user from the
+                # implementation detail of requests vs pycurl vs urllib
+                raise ServerError(url, -1, 'Request timed out after %s seconds' % timeout)
 
             # When the python-requests module gets a response, it attempts to
-            # guess the encoding using "charade", a fork of "chardet" which it
-            # bundles (and which we are in the process of unbundling:
-            # https://bugzilla.redhat.com/show_bug.cgi?id=910236).
+            # guess the encoding using chardet (or a fork)
             # That process can take an extraordinarily long time for long
             # response.text strings.. upwards of 30 minutes for FAS queries to
             # /accounts/user/list JSON api!  Therefore, we cut that codepath
Only in release/fedora: __init__.pyc
Only in release/fedora: release.pyc
diff -ur release/flask_fas_openid.py p/flask_fas_openid.py
--- release/flask_fas_openid.py	2013-04-01 11:28:08.516340400 -0700
+++ p/flask_fas_openid.py	2013-04-04 10:03:00.926567045 -0700
@@ -126,8 +126,8 @@
         :raises: Might raise an redirect to the OpenID endpoint
         """
         if return_url is None:
-            if 'next' in args.values:
-                return_url = args.values['next']
+            if 'next' in flask.request.args.values:
+                return_url = flask.request.args.values['next']
             else:
                 return_url = flask.request.url
         oidconsumer = consumer.Consumer(flask.session, None)

Attachment: pgpzetb8_1cRq.pgp
Description: PGP signature

_______________________________________________
infrastructure mailing list
infrastructure@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/infrastructure

[Index of Archives]     [Fedora Development]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]

  Powered by Linux