Change outgoing webhook timeout to 10s, and change the retry logic

Previous retry logic was:
- max 3 tries
- every try times out after 5 seconds

The new retry logic is:
- max 3 tries
- every try times out after 10 seconds
- if the first two tries have used > 10 seconds, don't
  do the third try

cc: #569

CHANGELOG.md

@@ -5,6 +5,7 @@ All notable changes to this project will be documented in this file.
 
 ### Improvements
 - Switch from croniter to cronsim (vendored in hc.lib.cronsim)
+- Change outgoing webhook timeout to 10s, but cap the total time to 20s
 
 ## v1.23.1 - 2021-10-13
 

hc/api/tests/test_notify_webhook.py

@@ -42,7 +42,7 @@ class NotifyWebhookTestCase(BaseTestCase):
             "get",
             "http://example",
             headers={"User-Agent": "healthchecks.io"},
-            timeout=5,
+            timeout=10,
         )
 
     @patch("hc.api.transports.requests.request", side_effect=Timeout)
@@ -143,7 +143,7 @@ class NotifyWebhookTestCase(BaseTestCase):
         self.assertEqual(args[0], "get")
         self.assertEqual(args[1], url)
         self.assertEqual(kwargs["headers"], {"User-Agent": "healthchecks.io"})
-        self.assertEqual(kwargs["timeout"], 5)
+        self.assertEqual(kwargs["timeout"], 10)
 
     @patch("hc.api.transports.requests.request")
     def test_webhooks_handle_variable_variables(self, mock_get):
@@ -205,7 +205,7 @@ class NotifyWebhookTestCase(BaseTestCase):
 
         url = "http://host/%24TAG1"
         mock_get.assert_called_with(
-            "get", url, headers={"User-Agent": "healthchecks.io"}, timeout=5
+            "get", url, headers={"User-Agent": "healthchecks.io"}, timeout=10
         )
 
     @patch("hc.api.transports.requests.request")
@@ -221,7 +221,7 @@ class NotifyWebhookTestCase(BaseTestCase):
         self.channel.notify(self.check)
 
         mock_get.assert_called_with(
-            "get", "http://bar", headers={"User-Agent": "healthchecks.io"}, timeout=5
+            "get", "http://bar", headers={"User-Agent": "healthchecks.io"}, timeout=10
         )
 
     @patch("hc.api.transports.requests.request")
@@ -271,7 +271,7 @@ class NotifyWebhookTestCase(BaseTestCase):
 
         headers = {"User-Agent": "healthchecks.io", "Content-Type": "application/json"}
         mock_request.assert_called_with(
-            "post", "http://foo.com", data=b"data", headers=headers, timeout=5
+            "post", "http://foo.com", data=b"data", headers=headers, timeout=10
         )
 
     @patch("hc.api.transports.requests.request")
@@ -288,7 +288,7 @@ class NotifyWebhookTestCase(BaseTestCase):
 
         headers = {"User-Agent": "healthchecks.io", "Content-Type": "application/json"}
         mock_request.assert_called_with(
-            "get", "http://foo.com", headers=headers, timeout=5
+            "get", "http://foo.com", headers=headers, timeout=10
         )
 
     @patch("hc.api.transports.requests.request")
@@ -305,7 +305,7 @@ class NotifyWebhookTestCase(BaseTestCase):
 
         headers = {"User-Agent": "My-Agent"}
         mock_request.assert_called_with(
-            "get", "http://foo.com", headers=headers, timeout=5
+            "get", "http://foo.com", headers=headers, timeout=10
         )
 
     @patch("hc.api.transports.requests.request")
@@ -325,7 +325,7 @@ class NotifyWebhookTestCase(BaseTestCase):
 
         headers = {"User-Agent": "healthchecks.io", "X-Message": "Foo is DOWN"}
         mock_request.assert_called_with(
-            "get", "http://foo.com", headers=headers, timeout=5
+            "get", "http://foo.com", headers=headers, timeout=10
         )
 
     @override_settings(WEBHOOKS_ENABLED=False)
@@ -376,4 +376,3 @@ class NotifyWebhookTestCase(BaseTestCase):
         args, kwargs = mock_request.call_args
 
         self.assertEqual(kwargs["headers"]["X-Foo"], "½")
-

hc/api/transports.py

@@ -1,4 +1,5 @@
 import os
+import time
 
 from django.conf import settings
 from django.template.loader import render_to_string
@@ -154,7 +155,7 @@ class HttpTransport(Transport):
     def _request(cls, method, url, **kwargs):
         try:
             options = dict(kwargs)
-            options["timeout"] = 5
+            options["timeout"] = 10
             if "headers" not in options:
                 options["headers"] = {}
             if "User-Agent" not in options["headers"]:
@@ -175,31 +176,28 @@ class HttpTransport(Transport):
             return "Connection failed"
 
     @classmethod
-    def get(cls, url, num_tries=3, **kwargs):
-        for x in range(0, num_tries):
-            error = cls._request("get", url, **kwargs)
-            if error is None:
-                break
+    def _request_with_retries(cls, method, url, use_retries=True, **kwargs):
+        start = time.time()
+        error = cls._request(method, url, **kwargs)
+        if error and use_retries:
+            for i in range(0, 2):
+                error = cls._request(method, url, **kwargs)
+                if error is None or time.time() - start > 10:
+                    break
 
         return error
 
     @classmethod
-    def post(cls, url, num_tries=3, **kwargs):
-        for x in range(0, num_tries):
-            error = cls._request("post", url, **kwargs)
-            if error is None:
-                break
-
-        return error
+    def get(cls, url, **kwargs):
+        return cls._request_with_retries("get", url, **kwargs)
 
     @classmethod
-    def put(cls, url, num_tries=3, **kwargs):
-        for x in range(0, num_tries):
-            error = cls._request("put", url, **kwargs)
-            if error is None:
-                break
+    def post(cls, url, **kwargs):
+        return cls._request_with_retries("post", url, **kwargs)
 
-        return error
+    @classmethod
+    def put(cls, url, **kwargs):
+        return cls._request_with_retries("put", url, **kwargs)
 
 
 class Webhook(HttpTransport):
@@ -254,17 +252,15 @@ class Webhook(HttpTransport):
         if body:
             body = self.prepare(body, check).encode()
 
-        num_tries = 3
-        if getattr(check, "is_test"):
-            # When sending a test notification, don't retry on failures.
-            num_tries = 1
+        # When sending a test notification, don't retry on failures.
+        use_retries = False if getattr(check, "is_test") else True
 
         if spec["method"] == "GET":
-            return self.get(url, num_tries=num_tries, headers=headers)
+            return self.get(url, use_retries=use_retries, headers=headers)
         elif spec["method"] == "POST":
-            return self.post(url, num_tries=num_tries, data=body, headers=headers)
+            return self.post(url, use_retries=use_retries, data=body, headers=headers)
         elif spec["method"] == "PUT":
-            return self.put(url, num_tries=num_tries, data=body, headers=headers)
+            return self.put(url, use_retries=use_retries, data=body, headers=headers)
 
 
 class Slack(HttpTransport):