ZhukV/AppleApnPush

[Feature request] Sending multiple requests without a foreach

morloderex opened this issue · 9 comments

Currently i'm looping through each of my device tokens in a foreach loop.

I would be great if there was some way that i could take this array with my prebuild message. And send it along by opening up only 1 connection instead of opening up 1 + n connections 1 foreach token to send to.

@ZhukV Can you collaborate on this?

ZhukV commented

@morloderex , you can use foreach (iterable functionality) to send the messages to devices. The connection (HTTP) does not close after success sends the message.

Connection closing if:

  • Shutdown application.
  • Fail to send the message (apple close connection after fail and we should reconnect).

You can see CurlHttpSender and HttpProtocol for more info.

From what i can see in CurlHttpSender we are opening up a new connection every single time i do something like.

// http builder logic here taken from documenation. 
$sender = $builder->build();

// this is an array of deviceTokens.
foreach($devices as $deviceToken)
{
    try {
        $sender->send(new Receiver(new DeviceToken($deviceToken, 'example.topc')), // message here, // environement here);
    catch(\Exception $e) { 
       // do something with the exception.
    }
}

With an array of 526 device tokens with 300 of those failing it took me 360s to run your code.

It would be way better if we could do it the other way around sudo code:

$sender = $builder->build();
$sender->connect();
// loop from above here

$sender->disconnect();

So we can control when the connection is opened and closed?

@ZhukV

ZhukV commented

Hm, interesting...

Yes, now we recreate the connection after fail sends the notification, but in the documentation of Apple Apn Push I read next:

Normal request failures do not result in termination of a connection.

As result, we should not recreate connection after fail to send the notification.

It would be way better if we could do it the other way around sudo code:

No, this is bad for single responsibility. The sender does not know about connection/protocol. Sender should have only one method - send the push notification.

@morloderex can you comment the functionality for recreate connection and testing this? How it work? Correct or not? Thank!

@ZhukV Forget SOLID for a second and think about performence if we recreate the connection every single time we send a push message. Regardless if an error has been thrown that's a big performence hit.

ZhukV commented

Yes, you right!!! We should search the average point between performance and easily develop/support. I think, what better solution for this problem - no close connection after fails to send the notification. But we should test this scenario. Now, I have the project with ~ 10K tokens, but on it project, I use the old version of the library (binary protocol).

If we shift our naming convention away from a sender class into some kind of connector class. Then we could have something like this (again sudo code)

$connector = $builder->build();
$connector->connect();
$connector->sendPush

$connector->disconnect();

If we do it in this way without breaking the connection and creating a new one every single time. We should have increased performence by a lot.

ZhukV commented

@morloderex please review the changes #45

After this changes, you should manually create the sender.

$protocol = $builder->buildProtocol();
$sender = new Sender($protocol);

// And close the connection
$protocol->closeConnection();

And please test this functionality for your case. Very thank!!!

I have added a comment inside the pr.

I am sorry i didn't came with any feedback on #45 . I found another solution for the issue. It's related to curl_exec() beeing really slow.

I can try recreating what we did ourselves with another pr if you want that.