jamesrgrinter
commented
9 years ago (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 ?
Jonathan-Gander
richsage
TheKassaK
Hello, I'm having trouble using this bundle in a production environment : some of my notifications are sent dozens of time to the same device. I'm using $this->container->get('rms_push_notifications')->send($message); in a 50 loops foreach. Everything works fine until a notification is detected as failed. Then the method sendMessages() calls itself recursively to resend to resend the failed messages, but it sends again those that succeded as well. Here is the recursive call I'm talking about : https://github.com/richsage/RMSPushNotificationsBundle/blob/master/Service/OS/AppleNotification.php#L146
Not sure if it is a misuse of the bundle, or an actual bug. Any help would be appreciated :)
Thank you