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 for BillingClientImpl Crashes - Promise consumed twice #379

Merged
merged 2 commits into from
Jan 25, 2019

Conversation

dajaffe
Copy link
Contributor

@dajaffe dajaffe commented Jan 24, 2019

It's quick and dirty but it seems to work for #315. I can't think of a scenario where you would ever want the same listener to hit it's onSetupFinished callback twice. If the app shutdown and restarted, and needed to get a new connection, it would have created a new listener. I also didn't see a way to unsubscribe a listener from the BillingClient.

@hyochan
Copy link
Owner

hyochan commented Jan 24, 2019

@mitchellmcm27 Could you review this? Looks like the approach is similar to PR rolled back. I feel like #315 and #374 is also related.

@hyochan hyochan added 🤖 android Related to android 🛠 bugfix All kinds of bug fixes labels Jan 24, 2019
@Override
public void onBillingSetupFinished(@BillingClient.BillingResponse int responseCode) {
if (responseCode == BillingClient.BillingResponse.OK ) {
Log.d(TAG, "billing client ready");
callback.run();
if (!bSetupCallbackConsumed) {
bSetupCallbackConsumed = true;
Copy link
Owner

Choose a reason for hiding this comment

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

I hope you to review the PR in #359 and check if this is really fixing the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the mBillingClient null check and isReady just moved the crash happening a little later. With 359's PR, that fixed where the old billing client was destroyed from the disconnect, but still attempted to be used. With that fix in place, it moved from trying to use an old client, to using a new one, but trying to hit the old promise a second time.

@hyochan
Copy link
Owner

hyochan commented Jan 25, 2019

Thank you for contribution. I will merge this commit since this looks pretty severe to try for any solution. Let's see how it goes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 android Related to android 🛠 bugfix All kinds of bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants