launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/set-timeout-on-redirect into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/set-timeout-on-redirect/+merge/157012

The last two checkwatches hangs have both been during XMLRPC requests, which made me quite suspicious. I thought I had fixed all of the callsites to set a timeout, which should stop the hangs. However, while reading code last night I noticed that the XMLRPC redirect handler creates a new request and does not copy the timeout set from the old request. Things like OpenerDirector.open will set the timeout specified on the request, and then make use of that inside urllib2. The problem with this code is that it will crash if there is a request object with no timeout. But this might be a blessing, since it should give us a traceback with information if so.
-- 
https://code.launchpad.net/~stevenk/launchpad/set-timeout-on-redirect/+merge/157012
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/set-timeout-on-redirect into lp:launchpad.

=== modified file 'lib/lp/bugs/externalbugtracker/xmlrpc.py'
--- lib/lp/bugs/externalbugtracker/xmlrpc.py	2013-02-11 01:08:12 +0000
+++ lib/lp/bugs/externalbugtracker/xmlrpc.py	2013-04-04 03:08:21 +0000
@@ -58,6 +58,7 @@
         # We can therefore just copy the data from the old request to
         # the new without worrying about breaking things.
         new_request.data = req.data
+        new_request.timeout = req.timeout
         return new_request
 
 

=== modified file 'lib/lp/bugs/tests/externalbugtracker-xmlrpc-transport.txt'
--- lib/lp/bugs/tests/externalbugtracker-xmlrpc-transport.txt	2011-02-08 21:01:56 +0000
+++ lib/lp/bugs/tests/externalbugtracker-xmlrpc-transport.txt	2013-04-04 03:08:21 +0000
@@ -114,8 +114,7 @@
 The UrlLib2Transport uses a custom HTTP redirection handler to handle
 redirect responses. This is a subclass of urllib2's HTTPRedirectHandler.
 
-    >>> from lp.bugs.externalbugtracker.xmlrpc import (
-    ...     XMLRPCRedirectHandler)
+    >>> from lp.bugs.externalbugtracker.xmlrpc import XMLRPCRedirectHandler
     >>> from urllib2 import HTTPRedirectHandler, Request
 
     >>> transport.opener.handlers
@@ -145,6 +144,7 @@
     ... """
     >>> request_to_be_redirected = Request(
     ...     'http://www.example.com', data=request_body)
+    >>> request_to_be_redirected.timeout = 30
 
     >>> handler = XMLRPCRedirectHandler()
     >>> redirected_request = handler.redirect_request(
@@ -173,11 +173,14 @@
         </param>
       </params>
     </methodCall>
+    >>> redirected_request.timeout == request_to_be_redirected.timeout
+    True
 
 If an XMLRPCRedirectHandler is passed a GET request to redirect, the new
 request will be a GET request with no payload.
 
     >>> request_to_be_redirected = Request('http://www.example.com')
+    >>> request_to_be_redirected.timeout = 30
     >>> redirected_request = handler.redirect_request(
     ...     request_to_be_redirected, None, 302, 'Moved', {},
     ...     newurl='http://www.example.com/redirected')