From 6158f9c5398bbbe6a389228d72467a47e4618a7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C4=93teris=20Caune?= Date: Thu, 14 Oct 2021 16:14:00 +0300 Subject: [PATCH] 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 | 1 + hc/api/tests/test_notify_webhook.py | 17 +++++----- hc/api/transports.py | 48 +++++++++++++---------------- 3 files changed, 31 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0766c062..68c1237e 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/hc/api/tests/test_notify_webhook.py b/hc/api/tests/test_notify_webhook.py index 156b6fe3..dfd8b424 100644 --- a/hc/api/tests/test_notify_webhook.py +++ b/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"], "½") - diff --git a/hc/api/transports.py b/hc/api/transports.py index 178b4a2b..43ecc57e 100644 --- a/hc/api/transports.py +++ b/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):