jamesmontemagno / InAppBillingPlugin

Cross-platform In App Billing Plugin for .NET
MIT License
651 stars 152 forks source link

BUG: iOS Transactions can remain open forever. Current call to FinishTransactions is unreliable. #30

Closed Bartmax closed 7 years ago

Bartmax commented 7 years ago

The only transactions that are finished right now are user initiated ones.

It's very easy (specially in development) to get unfinished transactions. Also the place in code where the transactions are finished is dangerous, because the library set the transaction as finished BEFORE the application actually did something with them.

the FinishTransaction call should be safer to be more like:

foreach (SKPaymentTransaction transaction in transactions)
{
    switch (transaction.TransactionState)
    {
        case SKPaymentTransactionState.Purchased:
            TransactionCompleted?.Invoke(transaction, true);
            SKPaymentQueue.DefaultQueue.FinishTransaction(transaction);
            break;
        case SKPaymentTransactionState.Failed:
            TransactionCompleted?.Invoke(transaction, false);
            SKPaymentQueue.DefaultQueue.FinishTransaction(transaction);
            break;
        default:
            break;
    }
}

and

public override void RestoreCompletedTransactionsFinished(SKPaymentQueue queue)
{
    // This is called after all restored transactions have hit UpdatedTransactions
    // at this point we are done with the restore request so let's fire up the event
    var rt = restoredTransactions.ToArray();
    // Clear out the list of incoming restore transactions for future requests
    restoredTransactions.Clear();

    TransactionsRestored?.Invoke(rt);

    foreach (var transaction in rt)
    {
        SKPaymentQueue.DefaultQueue.FinishTransaction(transaction);
    }
}
jamesmontemagno commented 7 years ago

For the first part where would that go?

For the second part I think I do that here: https://github.com/jamesmontemagno/InAppBillingPlugin/blob/master/src/Plugin.InAppBilling.iOS/InAppBillingImplementation.cs#L110

Bartmax commented 7 years ago

Sorry for not being clear. The first part is for the method public override void UpdatedTransactions(SKPaymentQueue queue, SKPaymentTransaction[] transactions)

https://github.com/jamesmontemagno/InAppBillingPlugin/blob/master/src/Plugin.InAppBilling.iOS/InAppBillingImplementation.cs#L289

For the any of those parts, that handler is not enough. You are creating this handler only for user initiated purchases/restores, (or to see it in another way, only for transactions the app know about). The are many other scenarios the method is called and you can't prepare or create a TaskCompletionSource. (failed to finalize, deferrer, autorenewal, etc). Those are never finalized. Even if 2 transactions with same productId starts at the same time, the TaskCompletionSource will return as soon as the first one is done and the other will never finalize.

To avoid any of this, one should not assume that the handler will be called for all transactions, hence why it's proper to have it outside the handler. There's no need to prepare a TaskCompletionSource because nothing is awaiting this method to finalize. Still one way to notify the application if any of those transaction went successful is desired. I specified that more in deep in https://github.com/jamesmontemagno/InAppBillingPlugin/issues/29

If you have an app using this library on a device for some time already, I suggest you to put a Debug.WriteLine(transaction.TransactionState) inside the foreach (SKPaymentTransaction transaction in transactions). You might be surprised by the output.

jamesmontemagno commented 7 years ago

So, for the first part I do call FinishTransaction whhen it is completed: https://github.com/jamesmontemagno/InAppBillingPlugin/blob/master/src/Plugin.InAppBilling.iOS/InAppBillingImplementation.cs#L187

Should I just remove that code?

Bartmax commented 7 years ago

As long as FinishTransaction is called on UpdatedTransactions, yes that code should be removed.

jamesmontemagno commented 7 years ago

Yeah, basically we now will finish the transaction when purchased or failed regardless if the TransactionCompleted is null or not. That is what this will fix up.

Bartmax commented 7 years ago

this code should be removed from the handler:

https://github.com/jamesmontemagno/InAppBillingPlugin/blob/master/src/Plugin.InAppBilling.iOS/InAppBillingImplementation.cs#L117-L118

it's already done on the RestoreTransactionCompleted, i think (not sure) it will throw an exception if you try to finish a finished transaction and with this change you are calling it twice for the same transaction.

jamesmontemagno commented 7 years ago

Done