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

[Feature request] Sending multiple requests without a foreach #44

Closed
morloderex opened this issue Dec 12, 2017 · 9 comments
Closed

[Feature request] Sending multiple requests without a foreach #44

morloderex opened this issue Dec 12, 2017 · 9 comments

Comments

@morloderex
Copy link

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
Copy link
Owner

ZhukV commented Dec 12, 2017

@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.

@morloderex
Copy link
Author

morloderex commented Dec 12, 2017

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
Copy link
Owner

ZhukV commented Dec 13, 2017

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!

@morloderex
Copy link
Author

@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
Copy link
Owner

ZhukV commented Dec 13, 2017

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).

@morloderex
Copy link
Author

morloderex commented Dec 13, 2017

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 added a commit that referenced this issue Dec 13, 2017
@ZhukV
Copy link
Owner

ZhukV commented Dec 13, 2017

@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!!!

@morloderex
Copy link
Author

I have added a comment inside the pr.

ZhukV added a commit that referenced this issue Dec 18, 2017
@ZhukV ZhukV closed this as completed Dec 18, 2017
@morloderex
Copy link
Author

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.

zhanghuid pushed a commit to zhanghuid/apns that referenced this issue Dec 5, 2023
zhanghuid pushed a commit to zhanghuid/apns that referenced this issue Dec 5, 2023
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