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

Fix #744: Bulk messages with aoiapns not working properly #746

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mikaelengstrom
Copy link

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.

Tagging @pomali who initially did the aoiapns integration in case you have any feedback.

@mikaelengstrom mikaelengstrom force-pushed the fix-bulk-messaging-for-aioapns branch 2 times, most recently from 5c24247 to 59ab2c6 Compare November 21, 2024 15:14
@mikaelengstrom
Copy link
Author

Would be thankful if for example @50-Course could look into this since you managed the last PR by @pomali

Copy link
Member

@50-Course 50-Course left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @mikaelengstrom. apologies, couldn't make a comment back yesterday - as the notification came in amid few things. i understand the intent behind this change, however, can we have a test suite to ensure this doesn't break things down the line?

@mikaelengstrom mikaelengstrom force-pushed the fix-bulk-messaging-for-aioapns branch from 59ab2c6 to 3f83045 Compare December 20, 2024 11:35
@mikaelengstrom
Copy link
Author

mikaelengstrom commented Dec 20, 2024

@50-Course I have now fixed the existing testsuite. In my opinion it is quite excessive already. Overall i have not changed interface that much, the difference should be that some more statuses is setting "inactive" than before. I also implemented APNSException on failures now to keep the async implementation closer to the original apns implementation. I switch from "status" to "message" since when doing bulk message we need to throw one exception and there can be multiple errors. I realize this is a breaking change, but afik this backend have not been released yet.

Regarding the actual issue i tried to resolve (faulty usage of async causing never exiting state on errors) i do not really see how to implement regression tests for that.

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 mikaelengstrom force-pushed the fix-bulk-messaging-for-aioapns branch from 3f83045 to 2933d36 Compare December 20, 2024 11:40
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

Successfully merging this pull request may close these issues.

3 participants