From 7dcb0a0d2884dc7f717815594324c0c31550f1d5 Mon Sep 17 00:00:00 2001 From: jr conlin Date: Fri, 21 Oct 2016 14:33:36 -0700 Subject: [PATCH] fix: improve handling of JSONResponseErrors have websocket log them instead of passing along to sentry and give them a metric closes #703 --- autopush/router/apns2.py | 13 ++++----- autopush/router/apnsrouter.py | 25 ++++++++++++++-- autopush/router/fcm.py | 7 +++++ autopush/router/gcm.py | 14 +++++++-- autopush/tests/test_router.py | 54 ++++++++++++++++++++++++++++++++--- 5 files changed, 96 insertions(+), 17 deletions(-) diff --git a/autopush/router/apns2.py b/autopush/router/apns2.py index 8eb182f7..85905f1f 100644 --- a/autopush/router/apns2.py +++ b/autopush/router/apns2.py @@ -122,16 +122,13 @@ def send(self, router_token, payload, apns_id, raise RouterException( "APNS Transmit Error {}:{}".format(response.status, reason), - status_code=500, + status_code=502, response_body="APNS could not process " - "your message {}".format(reason)) - except HTTP20Error as ex: + "your message {}".format(reason) + ) + except HTTP20Error: connection.close() - raise RouterException( - "APNS Processing error: {}".format(repr(ex)), - status_code=503, - response_body="APNS returned an error processing request", - ) + raise finally: # Returning a closed connection to the pool is ok. # hyper will reconnect on .request() diff --git a/autopush/router/apnsrouter.py b/autopush/router/apnsrouter.py index c19d1483..66069e82 100644 --- a/autopush/router/apnsrouter.py +++ b/autopush/router/apnsrouter.py @@ -1,8 +1,9 @@ """APNS Router""" import uuid -from twisted.logger import Logger +from hyper.http20.exceptions import ConnectionError, HTTP20Error from twisted.internet.threads import deferToThread +from twisted.logger import Logger from autopush.exceptions import RouterException from autopush.router.apns2 import ( @@ -131,9 +132,27 @@ def _route(self, notification, router_data): 'Mozilla Push')), content_available=1) apns_id = str(uuid.uuid4()).lower() + try: + apns_client.send(router_token=router_token, payload=payload, + apns_id=apns_id) + except ConnectionError: + self.ap_settings.metrics.increment( + "updates.client.bridge.apns.connection_err", + self._base_tags + ) + raise RouterException( + "Server error", + status_code=502, + response_body="APNS returned an error processing request", + log_exception=False, + ) + except HTTP20Error: + raise RouterException( + "Server error", + status_code=502, + response_body="APNS returned an error processing request", + ) - apns_client.send(router_token=router_token, payload=payload, - apns_id=apns_id) location = "%s/m/%s" % (self.ap_settings.endpoint_url, notification.version) self.ap_settings.metrics.increment( diff --git a/autopush/router/fcm.py b/autopush/router/fcm.py index 115a82a7..6e964ac4 100644 --- a/autopush/router/fcm.py +++ b/autopush/router/fcm.py @@ -1,6 +1,7 @@ """FCM Router""" import pyfcm +from requests.exceptions import ConnectionError from twisted.internet.threads import deferToThread from twisted.logger import Logger @@ -195,6 +196,12 @@ def _route(self, notification, router_data): except pyfcm.errors.AuthenticationError as e: self.log.error("Authentication Error: %s" % e) raise RouterException("Server error", status_code=500) + except ConnectionError as e: + self.metrics.increment("updates.client.bridge.fcm.connection_err", + self._base_tags) + self.log.warn("Could not connect to FCM server: %s" % e) + raise RouterException("Server error", status_code=502, + log_exception=False) except Exception as e: self.log.error("Unhandled FCM Error: %s" % e) raise RouterException("Server error", status_code=500) diff --git a/autopush/router/gcm.py b/autopush/router/gcm.py index d2850775..7928ea11 100644 --- a/autopush/router/gcm.py +++ b/autopush/router/gcm.py @@ -1,6 +1,7 @@ """GCM Router""" import gcmclient +from requests.exceptions import ConnectionError from twisted.internet.threads import deferToThread from twisted.logger import Logger @@ -121,6 +122,12 @@ def _route(self, notification, uaid_data): except gcmclient.GCMAuthenticationError as e: self.log.error("GCM Authentication Error: %s" % e) raise RouterException("Server error", status_code=500) + except ConnectionError as e: + self.log.warn("GCM Unavailable: %s" % e) + self.metrics.increment("updates.client.bridge.gcm.connection_err", + self._base_tags) + raise RouterException("Server error", status_code=502, + log_exception=False) except Exception as e: self.log.error("Unhandled exception in GCM Routing: %s" % e) raise RouterException("Server error", status_code=500) @@ -175,7 +182,9 @@ def _process_reply(self, reply, uaid_data, ttl, notification): "send message.", }) raise RouterException("GCM unable to deliver", status_code=410, - response_body="GCM recipient not available.") + response_body="GCM recipient not available.", + log_exception=False, + ) # retries: if reply.needs_retry(): @@ -185,7 +194,8 @@ def _process_reply(self, reply, uaid_data, ttl, notification): self._base_tags) raise RouterException("GCM failure to deliver, retry", status_code=503, - response_body="Please try request later.") + response_body="Please try request later.", + log_exception=False) self.metrics.increment("updates.client.bridge.gcm.succeeded", self._base_tags) diff --git a/autopush/tests/test_router.py b/autopush/tests/test_router.py index 95997ff9..00a8176c 100644 --- a/autopush/tests/test_router.py +++ b/autopush/tests/test_router.py @@ -145,6 +145,23 @@ def test_register_bad_channel(self): router_data={"token": "connect_data"}, app_id="unknown") + @inlineCallbacks + def test_connection_error(self): + from hyper.http20.exceptions import ConnectionError + + def raiser(*args, **kwargs): + raise ConnectionError("oops") + + self.router.apns['firefox'].connections[1].request = Mock( + side_effect=raiser) + + with assert_raises(RouterException) as e: + yield self.router.route_notification(self.notif, self.router_data) + + eq_(e.exception.response_body, 'APNS returned an error ' + 'processing request') + eq_(e.exception.status_code, 502) + @inlineCallbacks def test_route_notification(self): result = yield self.router.route_notification(self.notif, @@ -195,7 +212,7 @@ def test_bad_send(self): with assert_raises(RouterException) as ex: yield self.router.route_notification(self.notif, self.router_data) ok_(isinstance(ex.exception, RouterException)) - eq_(ex.exception.status_code, 500) + eq_(ex.exception.status_code, 502) eq_(ex.exception.message, 'APNS Transmit Error 400:boo') eq_(ex.exception.response_body, 'APNS could not process your ' 'message boo') @@ -209,9 +226,8 @@ def throw(*args, **kwargs): with assert_raises(RouterException) as ex: yield self.router.route_notification(self.notif, self.router_data) ok_(isinstance(ex.exception, RouterException)) - eq_(ex.exception.status_code, 503) - eq_(ex.exception.message, "APNS Processing error: " - "HTTP20Error('oops',)") + eq_(ex.exception.status_code, 502) + eq_(ex.exception.message, "Server error") eq_(ex.exception.response_body, 'APNS returned an error processing ' 'request') @@ -475,6 +491,21 @@ def check_results(fail): d.addBoth(check_results) return d + def test_router_notification_connection_error(self): + from requests.exceptions import ConnectionError + + def throw_other(*args, **kwargs): + raise ConnectionError("oh my!") + + self.gcm.send.side_effect = throw_other + self.router.gcm['test123'] = self.gcm + d = self.router.route_notification(self.notif, self.router_data) + + def check_results(fail): + self._check_error_call(fail.value, 502, "Server error") + d.addBoth(check_results) + return d + def test_router_notification_gcm_id_change(self): self.mock_result.canonical["old"] = "new" self.router.gcm['test123'] = self.gcm @@ -754,6 +785,21 @@ def check_results(fail): d.addBoth(check_results) return d + def test_router_notification_connection_error(self): + from requests.exceptions import ConnectionError + + def throw_other(*args, **kwargs): + raise ConnectionError("oh my!") + + self.fcm.notify_single_device.side_effect = throw_other + self.router.fcm = self.fcm + d = self.router.route_notification(self.notif, self.router_data) + + def check_results(fail): + self._check_error_call(fail.value, 502) + d.addBoth(check_results) + return d + def test_router_notification_fcm_id_change(self): self.mock_result['canonical_ids'] = 1 self.mock_result['results'][0] = {'registration_id': "new"}