[IOS - Recursive call] Method "sendMessages" sends dozens of notifications about rmspushnotificationsbundle HOT 5 OPEN

(This is timely: excuse my deconstructing the code "out loud". I started this trying to reassure myself that I'm not going to meet the same problem when I do something similar to you. But now, I've concluded that there is a bug here.)

It's a strange bit of code, and the reason for the recursion and the queue wasn't immediately obvious to me.

AppleNotification::send() is the only method that adds a message into the queue (the array $messages), but it also then immediately sends it with sendMessages().

If there's a failure, sendMessages() calls itself with (result['identifier'] + 1) to start at the "next" message in the queue. But how can there be a subsequent message in the queue when send() is the only method that can put one there, and is the only method (other than itself) that calls sendMessages()?

On first read-through, I thought it was adding one to the $lastMessageId when it calls itself, but it isn't. Not directly, anyway. It's taking the "Notification identifier" from Apple's server response, (which in turn is set by the sending party, i.e. this code), and adding 1 to that. As long as that value is actually increasing each time we should be fine... and where is this value set? Well, that's being set via send()'s call to createPayload(), which is where we're incrementing $lastMessageId.

Obviously in your case (and mine!) there's the one instance of the Symfony service rms_push_notifications.ios instantiating the AppleNotification class, which send() is being invoked on over and over. So it is building up that queue as it goes, and I can't see any way in which that could reset within the same instance of AppleNotification (though, if you called it a lot, it could wrap) whilst the queue didn't.

I believe this weird construction is because Apple's replies to the message sending is asynchronous with the submission of the messages. The Apple docs say "The binary interface is asynchronous". They also say "If you send a notification that is malformed or otherwise unintelligible, APNs returns an error-response packet and closes the connection. Any notifications that you sent after the malformed notification using the same connection are discarded, and must be resent".

So, I read this as saying that, if you keep sending messages at some point you may get back the error-response packet which tells you at which point your sending failed (and then closes down the TCP connection.)

When that happens, sendMessages() will commence sending from the subsequent message id to the one that failed, which is nice (although there's no way for the caller of send() to instantly know which one actually failed, and it's probably already moved onto the next one, it can get it - slightly inconveniently - via getResponses())

I think the bug is that it doesn't break out of the loop when the recursive call of sendMessages() returns. If there were multiple messages in the queue beyond the failed message then it could process them again (the recursive call) and then again on the return. Does that sound right? @richsage ?

from rmspushnotificationsbundle.

@jamesrgrinter I've made the same code analyze for myself. I think you're right.
I've two questions about your post :

1. Have you found a way to reproduce the bug ? I've try with 3 devices, the second has a wrong token. I've not encounter the issue.
2. Your solution would be to add break; right after the recusive call (or maybe after $errors[] = $result;). Does it work ?

I am also afraid that this bug append to me in a production environment. So I would like to find a way to correct it. But without reproducing the issue, it's difficult !

from rmspushnotificationsbundle.

@jamesrgrinter apologies for the delay in coming back to this, and thanks for taking the time to write your detailed response :-) I believe your analysis is correct, although I've not had chance to investigate further yet.

@Sigmanet15 did you manage to reproduce the issue?

Ping @rjmunro - could you also take a look at this and see what you think?

from rmspushnotificationsbundle.

@richsage sorry I've not reproduced this issue..

from rmspushnotificationsbundle.

Is this corrected now ? I think I have the same problem...

from rmspushnotificationsbundle.