Async consumption / transaction finalization

[alex-dorokhov]

In short
I believe it makes sense to add a new method to PurchaseManager, which has to be called by the application to mark the transaction as handled (instead of marking it automatically).

Motivation
Currently a PurchaseManager handles successful purchase in the following way:

1. It calls PurchaseObserver.handlePurchase
2. It marks transaction as completed (iOS) or consumes inapp (Google Play)

The step 2 is performed in the same thread as the step 1. In particular it also means, that if the step 1 is failed (application code), then the transaction is not marked as completed (which is the right behaviour).
The problem appears, when PurchaseObserver.handlePurchase is written in asynchronous way: to apply transaction the application needs to perform some long running operation (e.g. call to a database). In this case the only option is too trigger async operation and block the thread, which has 2 downsides:

1. The thread by itself can be a main thread (not sure though), so that could make the app unresponsive
2. That makes implementation of PurchaseObserver.handlePurchase not so convenient.

Proposal

1. Add new PurchaseManager.purchaseHandled(transaction) method (maybe you have better ideas for the name), which has be to called by the application code, once the purchase is successfully handled on the apps side.
2. For iOS PurchaseManager.purchaseHandled will call SKPaymentQueue.getDefaultQueue().finishTransaction (on the main thread)
3. For Android PurchaseManager.purchaseHandled will call mBillingClient.consumeAsync for CONSUMABLE purchases (on the main thread)
4. Add Transaction.systemTransaction (or systemObject) field to store underlaying system transaction object.
5. Remove finalisation of transactions from the current places.

The alternative to step 5 would be to add an option to PurchaseManagerConfig or an argument to PurchaseManager.install, something like: "boolean automaticTransactionFinalisation".

What do you think?

[MrStahlfelge]

I think you are right that the current handling could be improved. My remarks:

• Amazon implementation must be changed, too
• I think for remaining compatible to older versions and keep the complexity low for most games, handlePurchase should return a boolean value. If it returns true, the purchase is consumed/finished. If it is false (or an exception is thrown), it is not and handlePurchase must be called manually. That's close to your suggestion in the config, but more flexible as games can handle this more flexible this way.
• For this change, it would be good to update to Google Billing 2.0 and implement pending purchases which affects entitlements in a similar way.

[noblemaster]

You are making an interesting point.

1. if the handlePurchase method is indeed being called on the main thread, we potentially block the UI which is no good. Can we verify if this is really the case?
2. if it is not being called from the main thread, @MrStahlfelge's response is a lot more straightforward and easy to implement. I would just said we require an exception to be thrown within that method to prevent a purchase from being consumed. Sending the package to the server won't be a problem if it's outside the main thread, even if it takes a little while.
3. if it is being called on the main thread, we still use the same method as described in 2., but we internally make it so we call handlePurchase outside the main thread and handle it internally.

I think the goal is to make it easy for developers. It should be us that move things off the main thread as needed.

[MrStahlfelge]

mBillingClient.consumeAsync does not block the thread, it makes no difference on which thread it is called. Can't say something for iOS as I am not that familiar on how it works. Based on my impressions when using gdx-pay on iOS, It does not feel as if it blocks the main thread.

[alex-dorokhov]

We have to keep in mind, that users may assume (and it is actually convenient in most of the cases), that PurchaseObserver.handlePurchase is called on the LibGDX main thread (by the way it differs from the main thread on Android as far as remember), so they are free to modify any UI related object without conflicts.
Thus we come back to the original issue and it is not important, which thread is used by the OS.

[noblemaster]

handlePurchase should not be called using the libGDX main thread. Otherwise, sending the data to a server would block the UI as you correctly mentioned.

Updating the UI on the render thread is easy enough on libGDX (even if the calling thread is different). We just have to make it clear in the documentation.

[noblemaster]

@alex-dorokhov Maybe my message(s) are coming over a little hard? This isn't a criticism of your solution. Your solution is perfectly fine. The same goes with the existing solution which is simply a different way of going about things. It's a matter of taste and question of if we really want to change everything up for all the existing users?

Based on gdx-pay-example's MyFancyInAppShop.java, a developer has to assume the handlePurchase or handleRestore methods are called outside the main thread. Changing it to the main thread will cause problems, e.g. ANRs for developers who do server-side verification/other longer-running processes for purchase verification.

[alex-dorokhov]

@noblemaster, it's my fail, I thought it is called in the main thread in the current implementation.
Then you are right, we have to double check every implementation if the method is not called in the libGDX loop and not in a main loop of a platform UI. Maybe it makes sense to create a thread in the library anyway to be safe from future platform and libGDX changes.

About your second point:

2. if it is not being called from the main thread, @MrStahlfelge's response is a lot more straightforward and easy to implement. I would just said we require an exception to be thrown within that method to prevent a purchase from being consumed. Sending the package to the server won't be a problem if it's outside the main thread, even if it takes a little while.

I also like the solution by @MrStahlfelge, and I would apply it as it was originally suggested by him. Using a library should be both straightforward in async and sync style, so returning a boolean is better. Throwing an exception looks a bit "hacky" (IMHO).

The last sentence makes me think that you suggest to keep only the sync way of doing the things:

Sending the package to the server won't be a problem if it's outside the main thread, even if it takes a little while.

First of all, what you suggest with the exception, is currently implemented in the library. The purchase is not consumed if "handlePurchase" throws an exception. But the original issue is not only about blocking the calling thread, but also about convenience of implementation. Especially, when the code inside "handlePurchase" calls external async code.

Let me show you an example, where the "async" implementation is easier than the "sync" one. To simplify, I'm using the interface suggested by @MrStahlfelge.

Example. Using of Gdx.net.sendHttpRequest to send transaction info to the server.

The async way:

class AsyncExample implements PurchaseObserver {
    PurchaseManager purchaseManager;

    @Override
    public boolean handlePurchase(final Transaction transaction) {
        Gdx.net.sendHttpRequest(new Net.HttpRequest("https://server.com/purchase"), new Net.HttpResponseListener() {
            @Override
            public void handleHttpResponse(Net.HttpResponse httpResponse) {
                if (httpResponse.getStatus().getStatusCode() == HttpStatus.SC_ACCEPTED) {
                    purchaseManager.purchaseHandled(transaction);
                } else {
                    //TODO handle failure
                }
            }

            @Override
            public void failed(Throwable t) {
                //TODO handle failure
            }

            @Override
            public void cancelled() {
            }
        });

        return false;
    }
}

The sync way:

class SyncExample implements PurchaseObserver {
    PurchaseManager purchaseManager;

    @Override
    public boolean handlePurchase(final Transaction transaction) {
        final AtomicBoolean result = new AtomicBoolean(false);

        synchronized (result) {
            Gdx.net.sendHttpRequest(new Net.HttpRequest("https://server.com/purchase"), new Net.HttpResponseListener() {
                @Override
                public void handleHttpResponse(Net.HttpResponse httpResponse) {
                    if (httpResponse.getStatus().getStatusCode() == HttpStatus.SC_ACCEPTED) {
                        synchronized (result) {
                            result.set(true);
                            result.notify();
                        }
                    } else {
                        synchronized (result) {
                            result.set(false);
                            result.notify();
                        }
                    }
                }

                @Override
                public void failed(Throwable t) {
                    //TODO handle failure
                }

                @Override
                public void cancelled() {
                }
            });

            try {
                result.wait();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }

        return result.get();
    }
}

As you can see, the first solution is much easier to implement, because the user is not forced to use locks to convert async code (in this case Gdx.net, based on callbacks) to sync (gdx-pay).

In my personal case things go even harder, because it is not only required to send request to the server and wait for response, but additionally update the database (which happens in separate DB-thread).

[noblemaster]

Using a library should be both straightforward in async and sync style, so returning a boolean is better.

I agree, a boolean is the way to go I think. It's less hacky!

I also agree that the async way is what we should do. They synchronized(...) block won't work on the HTML backend anyway.

It's a little odd to always return false when sending the data to the server. On the other hand, it won't be a problem as the server can just ignore duplicate order IDs. That way, consumables also just don't disappear when they are consumed. It's safer to make sure people will always be able to restore their purchase, incl. consumable purchases in case the database would have to be restored. That way the purchases still appear (return true would not send them anymore when an order is restored).