Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

notification_failed not called on APNS error 8 #54

Closed
gdeglin opened this issue Jul 29, 2014 · 3 comments
Closed

notification_failed not called on APNS error 8 #54

gdeglin opened this issue Jul 29, 2014 · 3 comments
Milestone

Comments

@gdeglin
Copy link

gdeglin commented Jul 29, 2014

If a set of notifications fails to be delivered due to a connection error (For instance, on APNS error code 8), the notification_delivered reflection will still be run for all the notifications and the notification_failed reflection will never be called.

@gdeglin
Copy link
Author

gdeglin commented Jul 29, 2014

This also means that notification_delivered will get called even when the notifications following the failed one did not get delivered. It will get called repeatedly on each retry.

@gdeglin
Copy link
Author

gdeglin commented Jul 30, 2014

We want to keep a counter updated of notifications that have succeeded or failed so we worked around this by adding additional a couple additional reflections.

Since notification_delivered gets called even for notifications that we later discover we need to resend, we added an :apns_need_to_resend reflection to decrement the delivered count when any notifications get queued up again. We also make sure to call the notification_failed reflection when the notification failed (Normally this reflection wants the notification instance to be passed in, but since this value wasn't available in the apns_tcp class we used notification_id instead). Here's the commit: https://github.com/gdeglin/rpush/commit/413ad3fb378496158ed3bfd3e07cfebdd4118cd3

@ileitch
Copy link
Member

ileitch commented Aug 1, 2014

Thanks for the detailed report. Your commit looks like the right approach, i'll work on merging it (with some minor changes) this weekend.

@ileitch ileitch added this to the 2.0.0 milestone Aug 1, 2014
@ileitch ileitch closed this as completed in 1bda607 Aug 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants