diff --git a/zerver/lib/push_notifications.py b/zerver/lib/push_notifications.py index 7a1f564036..b2f367e4f8 100644 --- a/zerver/lib/push_notifications.py +++ b/zerver/lib/push_notifications.py @@ -15,6 +15,7 @@ from django.conf import settings from django.utils.timezone import now as timezone_now from django.utils.translation import ugettext as _ from gcm import GCM +from hyper.http20.exceptions import HTTP20Error import requests from six.moves import urllib import ujson @@ -64,6 +65,8 @@ def get_apns_client(): use_sandbox=settings.APNS_SANDBOX) return _apns_client +APNS_MAX_RETRIES = 3 + @statsd_increment("apple_push_notification") def send_apple_push_notification(user_id, devices, payload_data): # type: (int, List[DeviceToken], Dict[str, Any]) -> None @@ -74,12 +77,29 @@ def send_apple_push_notification(user_id, devices, payload_data): payload = APNsPayload(**payload_data) expiration = int(time.time() + 24 * 3600) client = get_apns_client() + retries_left = APNS_MAX_RETRIES for device in devices: # TODO obviously this should be made to actually use the async - stream_id = client.send_notification_async( - device.token, payload, topic='org.zulip.Zulip', - expiration=expiration) - result = client.get_notification_result(stream_id) + + def attempt_send(): + # type: () -> Optional[str] + stream_id = client.send_notification_async( + device.token, payload, topic='org.zulip.Zulip', + expiration=expiration) + try: + return client.get_notification_result(stream_id) + except HTTP20Error as e: + logging.warn("APNs: HTTP error sending for user %d to device %s: %s", + user_id, device.token, e.__class__.__name__) + return None + + result = attempt_send() + while result is None and retries_left > 0: + retries_left -= 1 + result = attempt_send() + if result is None: + result = "HTTP error, retries exhausted" + if result == 'Success': logging.info("APNs: Success sending for user %d to device %s", user_id, device.token) diff --git a/zerver/tests/test_push_notifications.py b/zerver/tests/test_push_notifications.py index 6a8a90b53c..f36c0ec9a7 100644 --- a/zerver/tests/test_push_notifications.py +++ b/zerver/tests/test_push_notifications.py @@ -1,6 +1,7 @@ from __future__ import absolute_import from __future__ import print_function +import itertools import requests import mock from mock import call @@ -526,6 +527,23 @@ class TestAPNs(PushNotificationTest): "APNs: Success sending for user %d to device %s", self.user_profile.id, device.token) + def test_http_retry(self): + # type: () -> None + import hyper + with mock.patch('zerver.lib.push_notifications._apns_client') as mock_apns, \ + mock.patch('zerver.lib.push_notifications.logging') as mock_logging: + mock_apns.get_notification_result.side_effect = itertools.chain( + [hyper.http20.exceptions.StreamResetError()], + itertools.repeat('Success')) + self.send() + mock_logging.warn.assert_called_once_with( + "APNs: HTTP error sending for user %d to device %s: %s", + self.user_profile.id, self.devices()[0].token, "StreamResetError") + for device in self.devices(): + mock_logging.info.assert_any_call( + "APNs: Success sending for user %d to device %s", + self.user_profile.id, device.token) + class TestGetAlertFromMessage(PushNotificationTest): def test_get_alert_from_message(self): # type: () -> None