push_notifications: Remove redundant APNs retry loop.

aioapns already has a retry loop.  By default it retries forever on
ConnectionError and ConnectionClosed, so our own retry loop would
never be reached.  Remove our retry loop, and configure aioapns to
retry APNS_MAX_RETRIES times on ConnectionError like the previous
version did.  It still retries forever on ConnectionClosed; that’s not
configurable but probably fine.

Signed-off-by: Anders Kaseorg <anders@zulip.com>

zerver/lib/push_notifications.py

@@ -88,6 +88,7 @@ def get_apns_context() -> Optional[APNsContext]:
     apns = aioapns.APNs(
         client_cert=settings.APNS_CERT_FILE,
         topic=settings.APNS_TOPIC,
+        max_connection_attempts=APNS_MAX_RETRIES,
         loop=loop,
         use_sandbox=settings.APNS_SANDBOX,
     )
@@ -157,52 +158,40 @@ def send_apple_push_notification(
     logger.info("APNs: Sending notification for user %d to %d devices", user_id, len(devices))
     payload_data = modernize_apns_payload(payload_data).copy()
     message = {**payload_data.pop("custom", {}), "aps": payload_data}
-    retries_left = APNS_MAX_RETRIES
     for device in devices:
         # TODO obviously this should be made to actually use the async
         request = aioapns.NotificationRequest(
             device_token=device.token, message=message, time_to_live=24 * 3600
         )
 
-        async def attempt_send() -> Optional[str]:
+        try:
-            assert apns_context is not None
+            result = apns_context.loop.run_until_complete(
-            try:
+                apns_context.apns.send_notification(request)
-                result = await apns_context.apns.send_notification(request)
+            )
-                return "Success" if result.is_successful else result.description
+        except aioapns.exceptions.ConnectionError as e:
-            except aioapns.exceptions.ConnectionClosed as e:  # nocoverage
+            logger.warning(
-                logger.warning(
+                "APNs: ConnectionError sending for user %d to device %s: %s",
-                    "APNs: ConnectionClosed sending for user %d to device %s: %s",
+                user_id,
-                    user_id,
+                device.token,
-                    device.token,
+                e.__class__.__name__,
-                    e.__class__.__name__,
+            )
-                )
+            continue
-                return None
-            except aioapns.exceptions.ConnectionError as e:  # nocoverage
-                logger.warning(
-                    "APNs: ConnectionError sending for user %d to device %s: %s",
-                    user_id,
-                    device.token,
-                    e.__class__.__name__,
-                )
-                return None
 
-        result = apns_context.loop.run_until_complete(attempt_send())
+        if result.is_successful:
-        while result is None and retries_left > 0:
-            retries_left -= 1
-            result = apns_context.loop.run_until_complete(attempt_send())
-        if result is None:
-            result = "HTTP error, retries exhausted"
-
-        if result == "Success":
             logger.info("APNs: Success sending for user %d to device %s", user_id, device.token)
-        elif result in ["Unregistered", "BadDeviceToken", "DeviceTokenNotForTopic"]:
+        elif result.description in ["Unregistered", "BadDeviceToken", "DeviceTokenNotForTopic"]:
-            logger.info("APNs: Removing invalid/expired token %s (%s)", device.token, result)
+            logger.info(
+                "APNs: Removing invalid/expired token %s (%s)", device.token, result.description
+            )
             # We remove all entries for this token (There
             # could be multiple for different Zulip servers).
             DeviceTokenClass.objects.filter(token=device.token, kind=DeviceTokenClass.APNS).delete()
         else:
             logger.warning(
-                "APNs: Failed to send for user %d to device %s: %s", user_id, device.token, result
+                "APNs: Failed to send for user %d to device %s: %s",
+                user_id,
+                device.token,
+                result.description,
             )
 
 

zerver/tests/test_push_notifications.py

@@ -1,7 +1,6 @@
 import asyncio
 import base64
 import datetime
-import itertools
 import re
 import uuid
 from contextlib import contextmanager
@@ -1373,60 +1372,6 @@ class TestAPNs(PushNotificationTest):
                     logger.output,
                 )
 
-    def test_http_retry(self) -> None:
-        import aioapns
-
-        self.setup_apns_tokens()
-        with self.mock_apns() as apns_context, self.assertLogs(
-            "zerver.lib.push_notifications", level="INFO"
-        ) as logger:
-            exception: asyncio.Future[object] = asyncio.Future(loop=apns_context.loop)
-            exception.set_exception(aioapns.exceptions.ConnectionError())
-            result = mock.Mock()
-            result.is_successful = True
-            future: asyncio.Future[object] = asyncio.Future(loop=apns_context.loop)
-            future.set_result(result)
-            apns_context.apns.send_notification.side_effect = itertools.chain(
-                [exception], itertools.repeat(future)
-            )
-            self.send()
-            self.assertIn(
-                f"WARNING:zerver.lib.push_notifications:APNs: ConnectionError sending for user {self.user_profile.id} to device {self.devices()[0].token}: ConnectionError",
-                logger.output,
-            )
-            for device in self.devices():
-                self.assertIn(
-                    f"INFO:zerver.lib.push_notifications:APNs: Success sending for user {self.user_profile.id} to device {device.token}",
-                    logger.output,
-                )
-
-    def test_http_retry_closed(self) -> None:
-        import aioapns
-
-        self.setup_apns_tokens()
-        with self.mock_apns() as apns_context, self.assertLogs(
-            "zerver.lib.push_notifications", level="INFO"
-        ) as logger:
-            exception: asyncio.Future[object] = asyncio.Future(loop=apns_context.loop)
-            exception.set_exception(aioapns.exceptions.ConnectionClosed())
-            result = mock.Mock()
-            result.is_successful = True
-            future: asyncio.Future[object] = asyncio.Future(loop=apns_context.loop)
-            future.set_result(result)
-            apns_context.apns.send_notification.side_effect = itertools.chain(
-                [exception], itertools.repeat(future)
-            )
-            self.send()
-            self.assertIn(
-                f"WARNING:zerver.lib.push_notifications:APNs: ConnectionClosed sending for user {self.user_profile.id} to device {self.devices()[0].token}: ConnectionClosed",
-                logger.output,
-            )
-            for device in self.devices():
-                self.assertIn(
-                    f"INFO:zerver.lib.push_notifications:APNs: Success sending for user {self.user_profile.id} to device {device.token}",
-                    logger.output,
-                )
-
     def test_http_retry_eventually_fails(self) -> None:
         import aioapns
 
@@ -1434,23 +1379,17 @@ class TestAPNs(PushNotificationTest):
         with self.mock_apns() as apns_context, self.assertLogs(
             "zerver.lib.push_notifications", level="INFO"
         ) as logger:
-            exception: asyncio.Future[object] = asyncio.Future(loop=apns_context.loop)
+            apns_context.apns.send_notification.return_value = asyncio.Future(
-            exception.set_exception(aioapns.exceptions.ConnectionError())
+                loop=apns_context.loop
-            apns_context.apns.send_notification.side_effect = iter([exception] * 5)
+            )
-
+            apns_context.apns.send_notification.return_value.set_exception(
+                aioapns.exceptions.ConnectionError()
+            )
             self.send(devices=self.devices()[0:1])
-            self.assert_length(
-                [log_record for log_record in logger.records if log_record.levelname == "WARNING"],
-                5,
-            )
             self.assertIn(
-                f"WARNING:zerver.lib.push_notifications:APNs: Failed to send for user {self.user_profile.id} to device {self.devices()[0].token}: HTTP error, retries exhausted",
+                f"WARNING:zerver.lib.push_notifications:APNs: ConnectionError sending for user {self.user_profile.id} to device {self.devices()[0].token}: ConnectionError",
                 logger.output,
             )
-            self.assert_length(
-                [log_record for log_record in logger.records if log_record.levelname == "INFO"],
-                1,
-            )
 
     def test_internal_server_error(self) -> None:
         self.setup_apns_tokens()