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
378 stars 121 forks source link

Limited number of simultaneous connections to prevent timeout errors #25

Closed remcoanker closed 6 years ago

remcoanker commented 6 years ago

Limited number of simultaneous connections to prevent timeout errors when sending 200+ notifications.

Array returned by push method now has device token as key.

ehuebner commented 6 years ago

I used the development branch (ECOMDevelopment:bugfix/connection-limit) in my project and it looks like there is still a bug. It worked well till ~700 recipients but with >1000 recipients the result array has not a response for every recipient. I suppose the stop criterion is not working properly and there is an edge case where it is stopped before the notification is send to all recipients.

I have a different implementation that is tested with >10000 recipients and i will provide it in a pull request this week.

remcoanker commented 6 years ago

I did notice some missing responses, but in my situation it was because I was sending two notifications to the same token in one request. The result array has the token as keys.

This can be fixed by using the input array index as identifier in CURLINFO_PRIVATE. This way the result array can also be the same order as the input, like it is in the current master.

Can you confirm this is not also your issue?

I'm looking forward to see your implementation.

ehuebner commented 6 years ago

Created a PR a week ago. Can you say when you have time to check it? I want get things done.

edamov commented 6 years ago

@ehuebner I left comment to your PR. Please check it.

ehuebner commented 6 years ago

@edamov i cannot see any comment.

edamov commented 6 years ago

@ehuebner Oh, I add comment but forgot to click "Submit review" button :man_facepalming: Check it now please

ahfeel commented 6 years ago

@remcoanker Your idea was quite good but did not solve all the concurrency issues (number of connections, properly closing handles, reusing of the global multi handle to reuse connections, etc.).

Unfortunately there was too many changes to just comment / review on this one, I made a fork and reworked quite a lot the Client.php file. See PR #29.

edamov commented 6 years ago

I close this PR because #29 solves all the concurrency issues