search

edamov / pushok

PHP client for Apple Push Notification Service (APNs) - Send push notifications to iOS using the new APNs HTTP/2 protocol with token-based (JWT with p8 private key)
MIT License
369 stars 119 forks source link

Fix for dropped notifications #48

Closed soren121 closed 5 years ago

soren121 commented 5 years ago

This fixes #35.

I have experienced the same issue in #35 where not all of my notifications were sent when I send hundreds of payloads at once. The issue report was cryptic though, and I investigated further to find out why that code sample posted by lipflip fixes the issue.

This problem is documented here and here. I am not a cURL expert but it seems that curl_multi_exec and curl_multi_select can unexpectedly return -1 if the function is called while cURL is doing something that can't be described to an application. This is more likely to happen when there are many handles waiting on the stack (or in other words, many notifications waiting to be sent.)

The only way to fix this is to wait for an arbitrary time and then try calling curl_multi_exec again. This is the solution that the PHP developers suggest, and it is also what the popular HTTP library Guzzle does.

To make the code more readable and easier to debug, I broke the large sending loop in Client.php into smaller loops and added comments. As an added benefit, this change should make pushok more efficient by blocking on curl_multi_select instead of eating CPU cycles in a loop. I also fixed running tests on PHP 7.2 so that I could run the test suite for this PR.

To test this pull request, I setup a mock APNS server on Amazon EC2 and sent 50,000 notification payloads in groups of 5,000 at a time. After confirming that it worked, I then tested on APNS Production servers with 50 notification payloads. No notifications were dropped.

soren121 commented 5 years ago

Tests failed on Travis because the Docker image does not have Git, and Coveralls expects Git to be available.

koenhoeijmakers commented 5 years ago

I have only limited knowledge of docker but would you mind creating a PR for https://github.com/edamov/pushok-docker too to add git which (if merged) will fix this PR?

Nice work btw!

soren121 commented 5 years ago

I think I can do that! I'll put in a PR for it soon.

soren121 commented 5 years ago

@edamov I submitted a PR to your pushok-docker repo about 3 weeks ago to fix the Travis build. Are you able to merge this sometime soon?

edamov commented 5 years ago

@soren121 I missed your PR. Now it is merged. But build still has an error. I will have time to investigate it only in a few days.

koenhoeijmakers commented 5 years ago

Looks like it still can't find git installed :thinking:

ahfeel commented 5 years ago

Up, could you fix the build so we can benefit from these pull requests ? :)

soren121 commented 5 years ago

Tests on Travis CI now pass. I took out the Docker image, it's not needed anymore. And it'll actually test on PHP 7.1 through 7.3 instead of whatever's latest in Alpine's repositories.

I no longer use this library in my projects as I personally don't feel like I can trust it. Hopefully though, this PR will help existing users of Pushok.

edamov commented 5 years ago

@soren121 Thanks a lot for your PR! It's cool we can drop that problematic docker image. Can I ask you what library do you use instead?

soren121 commented 5 years ago

You're welcome! Btw, no hard feelings, I know that as an open-source dev you are not obligated to provide anything. Switching was just a business decision.

I ended up writing my own library because I needed Android/FCM support too and it was more convenient to write one library that could send to APNS and FCM. Unfortunately, I don't think I am allowed to open-source it at the moment because of contracts and such...

eerison commented 5 years ago

What do you think put composer as global on docker image?

ahfeel commented 5 years ago

Wow, you guys are awesome, thank you so much for fixing this that fast !!

edamov commented 5 years ago

@eerison Why not? I think it is ok.

eerison commented 5 years ago

@eerison Why not? I think it is ok.

No, I'm not say no I did a suggestion for add composer as global because sometimes I want execute composer commands in another folders.