Web Push: Error when sending notification to unsubscribed browser

[nlittlejohns]

Got Web Push up and running pretty well now, but there seems to be one major flaw in my implementation: If someone unsubscribes from notifications, I get a 410 error and my production app shows an Internal Server Error if I try to send them a notification.

To reproduce:

• Register a browser for web push
• Unregister the service worker on the browser
• Trigger a Web Push notification to a set of devices which includes the browser which has unsubscribed

Result:

Internal Server Error: (URI)

WebPushError at (URI)
Push failed: 410 Gone
Response body:push subscription has unsubscribed or expired.

What's the best way to avoid this error? I'm planning to put my notification sending in a Celery task to make the site more responsive, which should hide the error from the user, but I'd like to avoid this error. Does it require a fix in django-push-notifications, or is there something I should be doing app-side?

(Perhaps @goinnn encountered this? Sorry for calling on you specifically!)

[jamaalscarlett]

I would catch the error and update the model to set the subscription as expired.

[nlittlejohns]

Thanks for your reply @jamaalscarlett! If I do this on a QuerySet of WebPush devices, the error is logged, but it appears the operation is halted before sending the notification to other (valid) devices in the QuerySet. Doing it this way also prevents me from discovering the ID of the unregistered device.

My code:

from push_notifications.models import APNSDevice, WebPushDevice
from push_notifications.webpush import WebPushError

...

    try:
        wp_devices.send_message(message=data)
    except WebPushError as e:
        print(e)

In this case, would it be best to iterate through the devices in the queryset and send notifications to them individually? something like:

    for device in wp_devices:
        try:
            device.send_message(message=data)
        except WebPushError as e:
            print(e)
            # remove device from db
            device.delete()

This code solves my problem perfectly, and the next time I run the same operation, the error doesn't occur (because the device has been deleted). I should probably add a bit of code which checks that the error is the one I'm expecting though!

It strikes me that it would perhaps be better to do this inside django-push-notifications? I'm happy to submit a PR if someone can point me to the right part of the code to implement it.

[jamaalscarlett]

Sorry as you can see from the contribution history, I haven't been active on this repo in years. What format is the error? Ideally we want to return a json object that enumerates which object caused the error. You could send the notification individually, that is become pretty inefficient as the queryset grows though.

[nlittlejohns]

This code is a pretty robust solution I think:

    for device in wp_devices:
        try:
            device.send_message(message=data)
        except WebPushError as e:
            result = re.search('{(.*)}', e.args[0])
            error = json.loads(result.group(0))
            print(error)
            if error['code'] in [404, 410]:
                # remove device from db
                device.delete()

It does the following:

1. Iterates through a queryset of Web Push devices
2. Tries to send a notification to each device (individually)
3. If there's an error for that device, it checks to see if the error code suggests that the device should not be tried again (see https://autopush.readthedocs.io/en/latest/http.html#error-codes)
4. If the error code is fatal (404 or 410), it removes the device from the DB so it's never tried again.

This works well from my app, but perhaps it should go into django-push-notifications somewhere?

[jamaalscarlett]

I agree, push_messages could return something like: { 'devices': device_count, 'success': success_count, 'error': error_count, 'errors': [ { 'device': device_id, 'error': { 'code': error_code, 'msg': error_msg } } ] }

[nlittlejohns]

Looks good to me, I'll try to investigate making a PR when I have a bit more time (which might not be until my workaround starts to get in the way!).

Hopefully my app-side workaround helps some folks in the mean time.

Thanks for your pointers @jamaalscarlett!