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

Added additional error handling for invalid apns device registration ids #877

Closed
wants to merge 3 commits into from

Conversation

AndrewFahlgren
Copy link

@AndrewFahlgren AndrewFahlgren commented Jun 20, 2018

Now when an error occurs during connection we are checking each notification for a bad device registration id and if we find any we re-queue the notifications and send InvalidToken errors for each one.

This code was adapted from code written by @ShalvaHMO here: #713

This code was written to solve issue: #713

…ids.

Now when an error occurs during connection we are checking each notification for a bad decive registration id and if we find any we requeue the notifications and send InvalidToken errors for each one.
@vladgaman
Copy link

This is not bad, but then again this method IsDeviceRegistrationIdValid() all is doing is checking it with a very simple regex: @"^[0-9A-F]+$" and this means if i have this registration id "AAA" then it will pass the regex thinking is a valid id, but in reality it is not.
Most likely you are going to get the right registration id anyway from APNS, in terms of this regex.

This method IsDeviceRegistrationIdValid() would have been a lot more useful if it actually checked if the registration id is an existing one.

@cvocvo cvocvo mentioned this pull request Jul 30, 2018
@chillNZ
Copy link

chillNZ commented Sep 3, 2018

Agreed with @vladgaman - this is an inadequate solution due to the fact ApnsNotification::ToBytes can raise a NotificationException for a number of reasons that aren't due to the regex failing such as:

  • Empty device token
  • One or more of the hex numbers in the device token is invalid
  • The device token length is less than 32
  • The payload is more than 2KB

In the cases above this code will get stuck in an infinite loop, attempting to resend the batch and failing over and over again. I've made a pull request #885 which simply handles the NotificationException in createBatch, calling the fail event and skipping the notifications which fail any of the possible validation exceptions from ToBytes, which avoids this issue.

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

Successfully merging this pull request may close these issues.

3 participants