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

Handling IllegalStateException in case of mutiple Buy method call #56

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

Florian-masse
Copy link

Possible fix for #54

Copy link
Contributor

@CyrilleGuimezanes CyrilleGuimezanes left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@jrouault
Copy link
Member

jrouault commented Jun 5, 2018

Hello,
Thank you for the fix! And sorry for the late answer.
A few possible improvements:

  • Ideally, not modifying the IabHelper.java would be preferable. It's considered as an "external" dependency. If we were to update it we would lose your changes, or have to apply them once again.
    A quick search pointed me to an updated version of this file there:
    https://github.com/googlesamples/android-play-billing/blob/master/TrivialDrive/app/src/main/java/com/example/android/trivialdrivesample/util/IabHelper.java
    Arguably, it hasn't been updated since a few years.
  • This seems to fix only the case of detecting a purchase started while another async operation is already happening. The exception happens for 3 async operations: consume, launchPurchaseFlow and refresh inventory.
    So for example, this fix would work if a "launchPurchaseFlow" was started while another "launchPurchaseFlow" is happening, which was the initial scenario in the issue.
    However, if a "consume" operation is started while another async operation is happening, the IllegalStateException exception would still be thrown.
  • Another error code than INVALID_TRANSACTION_STATE might be more appropriate. Something more general related to the fact that some async operation can not be run in parallel. However this would require changes to the Javascript-side as well.

@Florian-masse
Copy link
Author

Hello,
Thank you for the answer.

About those improvements:

  • I should have seen where IabHelper.java came from. It would be a whole new issue to use the updated class from your link but that only makes your point an even better one.
  • This just makes sense, two async calls to go.
  • Error codes are shared between the three parts of the plugin (Javascript, Java and Objective-C). Should not be a problem anyway.

Because the implementation would change otherwise , do you confirm the need of your first point over the second one ?

@jrouault
Copy link
Member

jrouault commented Jun 6, 2018

Just to make sure we are on the same page on the first point: I am not asking to update the file but simply not to modify it.
So yes, the first point has priority. But it should be possible to fix it in a way that makes the second point as easy to fix.

Copy link
Member

@jrouault jrouault left a comment

Choose a reason for hiding this comment

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

Sorry for the late review, and thanks for the changes!
It looks good. There's just a very small typo in the readme 😃

README.md Outdated
@@ -238,6 +238,7 @@ Failure callbacks return an error as an integer. See the following error table:
| 23 | `INVALID_PAYMENT` | Indicates that one of the payment parameters was not recognized |
| 24 | `UNAUTHORIZED` | Indicates that the user is not allowed to authorise payments (e.g. parental lock) |
| 25 | `RECEIPT_REFRESH_FAILED` | |
| 25 | `CONFLICTED_ASYNC_OPERATION`| Asynchronous calls cannot be run in parallel |
Copy link
Member

Choose a reason for hiding this comment

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

Very small typo here, it should be 26 :)

@Florian-masse
Copy link
Author

Here you go :)

@Florian-masse
Copy link
Author

Florian-masse commented Jun 12, 2018

Sorry, as my account is quite recent, github flagged it which hid everything I did on the platform.

@jrouault
Copy link
Member

No worries 😃 Thanks for fixing the last typo, I'll merge the PR asap!

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