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 idsAvailable not firing a 2nd time #563

Merged
merged 1 commit into from Jun 16, 2018
Merged

Conversation

jkasten2
Copy link
Member

@jkasten2 jkasten2 commented Jun 7, 2018


This change is Reviewable

@jkasten2 jkasten2 requested a review from Nightsd01 June 7, 2018 20:23
* Fixed issue where idsAvailable would not fire a 2nd time if registrationId the first time
@@ -1683,7 +1683,7 @@ public void run() {
runIdsAvailable.run();
}

private static void fireIdsAvailableCallback() {
static void fireIdsAvailableCallback() {

Choose a reason for hiding this comment

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

i commented in the github issue, but just curious. has this callback always been expected to fire twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but only if there was an issue with the device getting a registrationId. More details in my comment on the #563 PR.

@runningcode
Copy link

Also, do you have a repro case, so we can test on our side as well?

@runningcode
Copy link

Actually looking at this a second time. This doesn't seem to resolve the underlying issue.
If I understand correctly, registrationId and pushToken are the same thing.
This doesn't solve the problem that registrationId parameter in the idsavailable callback is null, even though OneSignal.getPermissionSubscriptionState().getSubscriptionStatus().getPushToken() is not null.

@jkasten2
Copy link
Member Author

@runningcode Thanks for reviewing the PR! I was not able to find any other case where the registrationId would be null when it shouldn't be.

This PR fixes the following case:

  1. First time the app start, OneSignal.startInit(...).endInit(); is called.
  2. Then app calls OneSignal.idsAvailable(idsAvailableCallback);
    . SDK tries to register for a FCM / GCM token.
  3. Device runs into an error registering the for a pushToken or times out.
    • SDK will keep trying but continues in the background while going to step 4.
  4. SDK then registers with onesignal.com to get a OneSignal playerId / userId
  5. After getting back a OneSignal userId the SDK fires callback;
    • idsAvailableCallback(userId: "UUID...", registrationId : null)
  6. Device finally gets a FCM / GCM pushToken, SDK fires callback a 2nd time;'
    • idsAvailableCallback(userId: "UUID...", registrationId : "fcmToken...")

This PR fixes the issue where step 7 was not happening.

The behavior of the 2nd fire in the case above has been in place for a while and has aways been the intended behavior. In most cases however, idsAvailableCallback will fire once with both values set when there are no timeout issues with FCM / GCM.

@runningcode
Copy link

We saw an issue in production where the callback is fired with a userId and a null registrationId idsAvailableCallback(userId: "UUID...", registrationId : null).

Inside of the callback we added some logging to track down the issue. userSubscription=true and subscribed=true.
We called OneSignal.getPermissionSubscriptionState().getSubscriptionStatus().getPushToken() and got a non-null pushToken.

Repeated app starts with excellent network conditions would lead to the exact same problem. We also asked our clients to reinstall the app and it led to the same problem. The OneSignal SDK was stuck in a state where the idsAvailableCallback would always fire with a null registrationId even though userSubscription=true and subscribed=true and getPushToken() returned a valid pushToken.

When would it make sense for the pushToken to return a value while the registrationId is null?
This workaround doesn't solve the issue we were seeing.

@jkasten2
Copy link
Member Author

@runningcode getPushToken() shouldn't be returning a non-null value while registrationId is. I'll take another pass at looking at the source and trying to reproduce the issue you described.

Going to move our conversation back to issue #558 since I want to keep this PR scoped to the title I opened this under.

@jkasten2 jkasten2 closed this Jun 16, 2018
@jkasten2 jkasten2 reopened this Jun 16, 2018
@jkasten2 jkasten2 merged commit cc67b06 into master Jun 16, 2018
@jkasten2 jkasten2 deleted the idsavailable_2nd_fire_fix branch June 16, 2018 06:13
@MacDegger
Copy link

Just to mention that we also got hit with this issue (after an update to OneSignal?) Anyway, since we didn't really use the pushToken/registrationid and were only looking for the userid, we replaced the call with OneSignal.getPermissionSubscriptionState().getSubscriptionStatus().getUserID(), which we could use instead ...

@runningcode
Copy link

@MacDegger curious, which version of onesignal were you on when this happened?

@MacDegger
Copy link

@runningcode we were on 3.8.4 and went to 3.9.3. Might it have to do with using a lambda version of the expression, ie: OneSignal.idsAvailable((userId, registrationId) -> {});?

@runningcode
Copy link

runningcode commented Jul 30, 2018

Ah, good to know. Would you mind commenting on this github issue? :) #558
It looks like you're seeing the same thing I saw.

3.9.3 should have the fix for the "ids not firing a second time" which was not related to the github issue I filed.
We kept the github issue open since I had a feeling this wouldn't resolve the underlying cause.

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.

None yet

4 participants