[Change Request] python-fedora two timeout fixes and a flask_fas_openid fix
Toshio Kuratomi
a.badger at gmail.com
Thu Apr 4 19:04:13 UTC 2013
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
-------------- next part --------------
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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.fedoraproject.org/pipermail/infrastructure/attachments/20130404/cd6611d9/attachment.sig>
More information about the infrastructure
mailing list