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

Issues with bulk messages in the new aioapns dependency (apns_async) #744

Open
mikaelengstrom opened this issue Oct 30, 2024 · 5 comments
Open

Comments

@mikaelengstrom
Copy link

mikaelengstrom commented Oct 30, 2024

Hi, i have been trying out the latest async version at scale (Im sending to 200k+ devices in production) and it does not really work for bulk messaging. Already around sending messages to 100 devices it justs hangs indefinitely and never terminates.

I have been trying to fix the issue, however i have zero experience with the underlying libraries (aioapns, h2, aiohttp) and dont really know how its supposed to work. But i managed to make it somewhat stable by using semaphores limiting the number of paralell requests and raising the connection limit. Implementation can be viewed here: mikaelengstrom@9a95da8

Unfortunatly, performance take quite a hit when done this way.

Very thankful if someone who knows this stuff better could check it out. The original contributor @pomali maybe? :)

@mikaelengstrom
Copy link
Author

I worked a bit further on this and it appears to be related to Fatal1ty/aioapns#41 (which in turn probably is related to another issue in h2). I did manage so send a several hundred thousand push notifications with this retry-logic: mikaelengstrom@ad9bd17

@mikaelengstrom
Copy link
Author

So digging deeper, one of the core issues was having non-legit registrationIds (such as BadDevice) in the device-list, making the connection overload and crash for some reason. But appart from that i had some issues with concurrency and the lib simply never terminating properly. I managed to develop a version where bulk-messaging works fast and performant as long as there are few or none non-legit registrationids: https://github.com/mikaelengstrom/django-push-notifications/blob/master/push_notifications/apns_async.py

This update does these main things:

  • Make sure everything regarding to aioapns happens in an async context and by doing so getting rid of the asyncio.get_eventloop-try-catch and instead just going for asyncio.run()
  • Setting devices to inactive on the failcodes "BadDeviceToken" and "DeviceTokenNotForTopic"
  • Integrated uvloop for blazing fast delivery

Any thoughts on this or should i just create a PR?

@pomali
Copy link
Contributor

pomali commented Nov 4, 2024

Nice catch! I think creating PR is a good idea, and we can sort out issues inside the PR (tag me too pls). (Btw don't make same mistake as me creating PR from your master branch, it gets messy with permissions if I remember correctly)

Is there some additional work to be done in aioapns or h2?

@HashimJVZ
Copy link

@pomali I have added support for mutable-content in aioapns #745 . It was available in apns2, Don't know if it is deliberately removed or missed when replacing **kwargs with fixed parameters.

mikaelengstrom added a commit to mikaelengstrom/django-push-notifications that referenced this issue Nov 21, 2024
Previous version implemented aioapns in a way that made
it hang indefinetly. Especially when receiver list contaned
a lot of bad tokens.

Having a lot of bad tokens still affects the reliability and
transfer speed of notification sets, which is why this fix
also deactivate devices for error codes BadDeviceToken and
DeviceTokenNotForTopic unlike previous versions.
@mikaelengstrom
Copy link
Author

@pomali sorry for late reply, i have been very busy. I have now opened a PR #746 - I do think there is some further work to be done in aoiapns/h2 or its sub-dependencies for 100% reliablity, however im unable to pinpoint what, but there are still some SSL errors going on when the connections are going super warm. In my pretty extensive testing the delivery rate is way above 99.9% though.

@HashimJVZ - I think it would be great if your patch was applied after my more excessive refactor of the module.

mikaelengstrom added a commit to mikaelengstrom/django-push-notifications that referenced this issue Nov 21, 2024
Previous version implemented aioapns in a way that made
it hang indefinetly. Especially when receiver list contaned
a lot of bad tokens.

Having a lot of bad tokens still affects the reliability and
transfer speed of notification sets, which is why this fix
also deactivate devices for error codes BadDeviceToken and
DeviceTokenNotForTopic unlike previous versions.
mikaelengstrom added a commit to mikaelengstrom/django-push-notifications that referenced this issue Nov 21, 2024
Previous version implemented aioapns in a way that made
it hang indefinetly. Especially when receiver list contaned
a lot of bad tokens.

Having a lot of bad tokens still affects the reliability and
transfer speed of notification sets, which is why this fix
also deactivate devices for error codes BadDeviceToken and
DeviceTokenNotForTopic unlike previous versions.
mikaelengstrom added a commit to mikaelengstrom/django-push-notifications that referenced this issue Dec 20, 2024
Previous version implemented aioapns in a way that made
it hang indefinetly. Especially when receiver list contaned
a lot of bad tokens.

Having a lot of bad tokens still affects the reliability and
transfer speed of notification sets, which is why this fix
also deactivate devices for error codes BadDeviceToken and
DeviceTokenNotForTopic unlike previous versions.
mikaelengstrom added a commit to mikaelengstrom/django-push-notifications that referenced this issue Dec 20, 2024
Previous version implemented aioapns in a way that made
it hang indefinetly. Especially when receiver list contaned
a lot of bad tokens.

Having a lot of bad tokens still affects the reliability and
transfer speed of notification sets, which is why this fix
also deactivate devices for error codes BadDeviceToken and
DeviceTokenNotForTopic unlike previous versions.
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

3 participants