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

[android] make sure devicePushToken AsyncCondition is always resolved properly #2661

Merged
merged 1 commit into from Nov 12, 2018

Conversation

esamelson
Copy link
Contributor

@esamelson esamelson commented Nov 8, 2018

Why

Fix for #2381 . It's currently possible to get in a state where the AsyncCondition for devicePushToken is resolved before the conditions in isReady in the method to get the Expo push token are met. In this case, the promise from the getExpoPushTokenAsync method never gets resolved or rejected.

How

Changed the isReady condition and the location of the AsyncCondition.notify calls so this state is no longer possible.

Also ensured that even if errors happen in the body of the onHandleIntent method, AsyncCondition.notify will still be called => the promise will be rejected instead of hanging.

Test Plan

Built a standalone app that logs the Expo push token immediately upon launch. Before this change, the promise would never resolve ~75% of the time on a fresh install. After the change, it has resolved 10/10 times I tested.

@esamelson esamelson requested a review from ide November 8, 2018 21:59
@esamelson esamelson added this to the M34 milestone Nov 8, 2018
Copy link
Member

@ide ide left a comment

Choose a reason for hiding this comment

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

This looks better than before but I think the behavior is still suboptimal in that the first boot still requires the updatePushToken request to succeed (not a regression, just that we'll still want to revisit this during the SDK audit).

@esamelson
Copy link
Contributor Author

ok, that makes sense. I think this fixes the main cause of #2381, however, so I'm going to merge this now so we can get it into a bugfix turtle release ASAP.

@esamelson esamelson merged commit 567dd98 into master Nov 12, 2018
@esamelson esamelson deleted the @eric/fcm-fix branch November 30, 2018 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants