[in_app_purchase] Write to the transactions update queue from the main thread

[edpizzi]

Equeue to the channel from the main thread to avoid channel safety issues.

Background: sendTransactionUpdate can be called from the main thread, where it is safe to send messages on the onTransactionsUpdated channel. This seems to happen when the purchase code path succeeds and generates a transaction. However sendTransactionUpdate can also be called on non-main threads serving background executors, when transaction callbacks result from restoring purchases or other transactions are reported to the transaction subscription (eg. a purchase made outside of the app, or a refund is issued -- this can be triggered from XCode's StoreKit Test transaction log). In these cases, at least sometimes, an error is logged about writing to a channel on a non-platform thread (see the issue below).

Enqueue on the main thread to avoid channel thread-safety issues. This could be optimized further, but I believe this is safe.

Resolves flutter/flutter#166493

Pre-Review Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter.
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I linked to at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under¹.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under¹.
• I updated/added any relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or I have commented below to indicate which test exemption this PR falls under¹.
• All existing and new tests are passing.

Footnotes

1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩² ↩³

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

[edpizzi]

Hi @LouiseHsu -- I'm not able to figure out how to test this on my own. With a small change to testRestoreProductSuccess (waiting for purchaseExpectation before triggering restorePurchases), I can demonstrate a callback that comes in on a non-main thread, the error condition here. However in the unit test transactionCallbackAPI is null since we use a constructor for the plugin that does not set up a channel, so the error condition does not occur -- we don't send a message to a channel from a non-main thread, since we don't send a message at all and there is no channel.

However the reason the error is avoided is synthetic, and is specific to the unit test setup. I think testing this would require changing how the tests are structured, which I'm not sure I can take on, and either way would require maintainer guidance (i.e., why are the tests set up this way, and what is the cost / complexity of changing it to include a message channel). Even then, the success criterion would be "log message does not occur", which wouldn't cause the test to fail I don't think (unless there's a debug assert in there; I haven't seen one), so we'd have to look at the test output, unless there's a log sink we can inspect.

In any case, I think the fix is quite important, while figuring out a test strategy is less important, and I don't think it's wise to gate a fix on (unless it's disputed whether this is a crash risk). But that's just my opinion. Either way, if we gate on a test strategy, I'll require some help, and it might be better handed off to someone with domain expertise.

(There's some relevant discussion in #hackers-new in the discord as I've been trying to figure this out, but the important bits are contained here.)

[edpizzi]

Ok, I implemented a testing strategy where we create a fake InAppPurchase2CallbackAPIProtocol that just asserts that it's called on the main thread, and set it on the plugin in InAppPurchase2PluginTests. Tests fail before the fix, and pass after.

[LouiseHsu]

Looks good to me! I think the failed test is flaky, Im just going to rerun it, and if it passes ill approve this :)

[LouiseHsu]

LGTM! Thanks for fixing this <3

[auto-submit]

autosubmit label was removed for flutter/packages/9068, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

• Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

[LouiseHsu]

oh oops. I forgot about the 2 reviewer rule. Tagging @hellohuanlin for secondary review

[edpizzi]

Friendly ping, @hellohuanlin, please and thank you.

@LouiseHsu - what's the release schedule for in_app_purchase_storekit? Developers probably shouldn't enable StoreKit2 until this is released. Is there a target date for enabling StoreKit2 by default?

[LouiseHsu]

Friendly ping, @hellohuanlin, please and thank you.

@LouiseHsu - what's the release schedule for in_app_purchase_storekit? Developers probably shouldn't enable StoreKit2 until this is released. Is there a target date for enabling StoreKit2 by default?

I'm aiming to enable Storekit2 sometime before the end of the month, but there's no strict schedule. There's a couple other contributer PRs I would like to land first as well.

[edpizzi]

Resolving a version number merge conflict. I assume I keep this as 0.3.22+1 unless there will be a release of some kind (vs. 0.3.23)?

[hellohuanlin]

nit: can you do

Task { @MainActor in 
  ....
}

To be consistent with other parts of the code?

[edpizzi]

Done. I'm afraid I see tests failing at main (testGetTransactionJsonRepresentation, testFinishTransaction, at bfb42a6). I assume it'll get sorted out.