launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-1489792 into lp:launchpad.

Commit message:
Turn webhook ReadTimeouts and ProxyErrors into connection_errors rather than OOPSes.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1489792 in Launchpad itself: "WebhookClient.deliver crashes if the endpoint times out"
  https://bugs.launchpad.net/launchpad/+bug/1489792

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1489792/+merge/269716

Turn webhook ReadTimeouts and ProxyErrors into connection_errors rather than OOPS.es

We really want to extract the X-Squid-Error from HTTPS ProxyErrors eventually, but it's not possible without some httplib hackery.

-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1489792 into lp:launchpad.

=== modified file 'lib/lp/services/webhooks/client.py'
--- lib/lp/services/webhooks/client.py	2015-08-24 10:22:58 +0000
+++ lib/lp/services/webhooks/client.py	2015-09-01 08:18:14 +0000
@@ -83,10 +83,15 @@
                 'body': preq.body,
                 },
             }
+        connection_error = None
         try:
             resp = session.send(preq, proxies=proxies, timeout=timeout)
-        except requests.ConnectionError as e:
-            result['connection_error'] = str(e)
+        except (requests.ConnectionError, requests.exceptions.ProxyError) as e:
+            connection_error = str(e)
+        except requests.exceptions.ReadTimeout:
+            connection_error = 'Request timeout'
+        if connection_error is not None:
+            result['connection_error'] = connection_error
             return result
         # If there was a request error, try to interpret any Squid
         # error.

=== modified file 'lib/lp/services/webhooks/tests/test_job.py'
--- lib/lp/services/webhooks/tests/test_job.py	2015-08-24 01:19:02 +0000
+++ lib/lp/services/webhooks/tests/test_job.py	2015-09-01 08:18:14 +0000
@@ -16,6 +16,7 @@
     )
 from pytz import utc
 import requests
+import requests.exceptions
 from storm.store import Store
 from testtools import TestCase
 from testtools.matchers import (
@@ -213,6 +214,19 @@
                 }))
         self.assertEqual([], reqs)
 
+    def test_timeout_error(self):
+        # Attempts that don't return within the timeout have a
+        # connection_error rather than a response.
+        reqs, result = self.sendToWebhook(
+            raises=requests.exceptions.ReadTimeout())
+        self.assertThat(
+            result,
+            MatchesDict({
+                'request': self.request_matcher,
+                'connection_error': Equals('Request timeout'),
+                }))
+        self.assertEqual([], reqs)
+
     def test_proxy_error_known(self):
         # Squid error headers are interpreted to populate
         # connection_error.