[in_app_purchase_android] Implement `BillingClient` connection management and introduce `BillingClientManager` #3303

SynSzakala · 2023-02-26T11:14:37Z

This PR adds management of BillingClient connection to Android implementation of InAppPurchase. This should fix issues caused by lost connections.

New and previously existing connection management is extracted to a new component, BillingClientManager.
This component is exported in billing_client_wrappers.dart, so that it can also be used when interacting with BillingClient directly.

Fixes flutter/flutter#110663

Migrated from flutter/plugins PR #6309

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…econnect # Conflicts: # packages/in_app_purchase/in_app_purchase_android/CHANGELOG.md # packages/in_app_purchase/in_app_purchase_android/pubspec.yaml

SynSzakala · 2023-02-26T11:18:25Z

@gmackall, @bparrishMines re-requesting review after migrating this PR. Hope we can get this merged soon!

bparrishMines

Thanks for the contribution!

bparrishMines · 2023-03-01T21:48:59Z

...purchase/in_app_purchase_android/lib/src/billing_client_wrappers/billing_client_manager.dart

+  }
+
+  bool _debugAssertNotDisposed() {
+    assert(() {


I don't understand why one would throw a FlutterError inside of an assert. It defeats the point of having an assert. Would it not be better to make it this:

assert(!_isDisposed, 'my error message');

bparrishMines · 2023-03-01T21:51:17Z

...purchase/in_app_purchase_android/lib/src/billing_client_wrappers/billing_client_manager.dart

+  Future<R> run<R extends HasBillingResponse>(
+    Future<R> Function(BillingClient client) block,
+  ) async {
+    assert(_debugAssertNotDisposed());


This is running an assert for a method that runs an assert. I think you could just have:

_debugAssertNotDisposed();

bparrishMines · 2023-03-01T21:51:28Z

...purchase/in_app_purchase_android/lib/src/billing_client_wrappers/billing_client_manager.dart

+  /// See [runRaw] for operations that return a subclass
+  /// of [HasBillingResponse].
+  Future<R> runRaw<R>(Future<R> Function(BillingClient client) block) async {
+    assert(_debugAssertNotDisposed());


Same for here for removing the assert.

bparrishMines · 2023-03-01T21:58:42Z

...purchase/in_app_purchase_android/lib/src/billing_client_wrappers/billing_client_manager.dart

+  /// See [runRaw] for operations that do not return a subclass
+  /// of [HasBillingResponse].
+  Future<R> run<R extends HasBillingResponse>(
+    Future<R> Function(BillingClient client) block,


This parameter could use a more descriptive name. Callback methods in Dart are typically given a name such as on<Action>. My assumption is that this could be called onBillingClientAvailable or onBillingClientReady.

I don't think that'd be a good choice, since function that's passed here isn't a callback, it's called in place (across an optional asynchronous gap). This is more similar to Flutter's setState with argument named just fn. I agree that block is a bit ambiguous, what do you think about action instead?

That's a good point. action works for me!

bparrishMines · 2023-03-01T21:58:58Z

...purchase/in_app_purchase_android/lib/src/billing_client_wrappers/billing_client_manager.dart

+  ///
+  /// See [runRaw] for operations that return a subclass
+  /// of [HasBillingResponse].
+  Future<R> runRaw<R>(Future<R> Function(BillingClient client) block) async {


Same with this block parameter.

bparrishMines · 2023-03-01T22:12:46Z

packages/in_app_purchase/in_app_purchase_android/lib/src/in_app_purchase_android_platform.dart

+        ),
+        skuDetailsList: const <SkuDetailsWrapper>[],
+      );
+      responses = <SkuDetailsResponseWrapper>[response, response];


Nice, this is good cleanup.

nit: Could you add a short comment above why this is leaving two identical responses. It wasn't immediately clear that they correspond with each of the queries done above.

bparrishMines · 2023-03-01T22:21:10Z

...purchase/in_app_purchase_android/lib/src/billing_client_wrappers/billing_client_manager.dart

+  ///
+  /// See [runRaw] for operations that do not return a subclass
+  /// of [HasBillingResponse].
+  Future<R> run<R extends HasBillingResponse>(


I think this method name could be more descriptive. As in it could be runWhenClientIsReady or runOnClientAvailable. I think this would make it easier to understand when this method is being used in the code. Same goes for the method runRaw.

I also agree that run/runRaw is ambiguous. I don't think that runWhenClientIsReady, runOnClientAvailable would be good though. Main idea of BillingClientManager is to handle BillingClient connection transparently, so its user doesn't have to worry about client readiness/availability/retries. I suggest withClient and withClientNonRetryable instead.

How about runWithClient?

Sure, done.

bparrishMines · 2023-03-01T22:24:09Z

...purchase/in_app_purchase_android/lib/src/billing_client_wrappers/billing_client_manager.dart

+  bool _isDisposed = false;
+
+  // Initialized immediately in the constructor, so it's always safe to access.
+  late Future<void> _readyFuture;


nit: This is optional, but a Completer<void> could be used instead of a Future<void>. This would make it so you would not have to depend on a late initializer.

In this case this wouldn't make much difference - new Completer would always have to be created in _connect function anyway.

…ientManager

…econnect

…ientManager

bparrishMines

It looks like you need to pull in the latest changes and run the formatter. Everything else looks good to me besides a few nits.

cc @stuartmorgan for a secondary review

bparrishMines · 2023-03-09T21:10:35Z

...purchase/in_app_purchase_android/lib/src/billing_client_wrappers/billing_client_manager.dart

+  /// Consider calling [dispose] after you no longer need the [BillingClient]
+  /// API to free up the resources.
+  ///
+  /// After calling [dispose] :


nit:

Suggested change

/// After calling [dispose] :

/// After calling [dispose]:

bparrishMines · 2023-03-09T21:10:54Z

...purchase/in_app_purchase_android/lib/src/billing_client_wrappers/billing_client_manager.dart

+  /// API to free up the resources.
+  ///
+  /// After calling [dispose] :
+  /// - Further connection attempts will not be made;


nit: To be consistent

Suggested change

/// - Further connection attempts will not be made;

/// - Further connection attempts will not be made.

bparrishMines · 2023-03-09T21:11:10Z

...purchase/in_app_purchase_android/lib/src/billing_client_wrappers/billing_client_manager.dart

+  ///
+  /// After calling [dispose] :
+  /// - Further connection attempts will not be made;
+  /// - [purchasesUpdatedStream] will be closed;


nit:

Suggested change

/// - [purchasesUpdatedStream] will be closed;

/// - [purchasesUpdatedStream] will be closed.

bparrishMines · 2023-03-09T21:15:32Z

...purchase/in_app_purchase_android/lib/src/billing_client_wrappers/billing_client_manager.dart

+  /// In order to access the [BillingClient], consider using [runWithClient]
+  /// and [runWithClientNonRetryable]
+  /// methods.


Since this is only visible for testing, it should recommend only using these methods.

nit:

Suggested change

/// In order to access the [BillingClient], consider using [runWithClient]

/// and [runWithClientNonRetryable]

/// methods.

/// In order to access the [BillingClient], use [runWithClient]

/// or [runWithClientNonRetryable] methods.

…_android_billing_client_reconnect # Conflicts: # packages/in_app_purchase/in_app_purchase_android/CHANGELOG.md # packages/in_app_purchase/in_app_purchase_android/pubspec.yaml

bparrishMines

LGTM

bparrishMines · 2023-03-15T21:16:04Z

packages/in_app_purchase/in_app_purchase_android/CHANGELOG.md

@@ -1,3 +1,8 @@
+## 0.2.5
+
+* Fixes the management of `BillingClient` connection.


nit: This should include how the management is being fixed.

bparrishMines · 2023-03-15T21:19:06Z

site-shared

I'm confident this shouldn't be changed, but I'm not familiar with submodules. Can you remove this diff?

SynSzakala · 2023-03-17T09:13:16Z

Done! Also ping @stuartmorgan on this review.

bparrishMines

LGTM

cc @stuartmorgan for secondary review

…econnect

tarrinneal

This seems solid to me. I think it would be best to wait for @stuartmorgan to add his review before merging. If he doesn't, and you'd like to merge here is mine to allow for that as well.

SynSzakala · 2023-04-06T08:53:21Z

@tarrinneal I think we actually might go forward and merge this, since @stuartmorgan already gave his approval before the migration.

bparrishMines · 2023-04-10T12:59:52Z

This was already approved in flutter/plugins#6309 and I was essentially giving this a secondary review. So this should be fine to submit for @tarrinneal and I.

…n management and introduce `BillingClientManager` (flutter/packages#3303)

…ment and introduce `BillingClientManager` (flutter#3303) [in_app_purchase_android] Implement `BillingClient` connection management and introduce `BillingClientManager`

Recreating PR from flutter/plugins

bf467c0

SynSzakala requested a review from gmackall as a code owner February 26, 2023 11:14

github-actions bot added p: in_app_purchase platform-android labels Feb 26, 2023

Merge branch 'main' into fix_in_app_purchase_android_billing_client_r…

362756d

…econnect # Conflicts: # packages/in_app_purchase/in_app_purchase_android/CHANGELOG.md # packages/in_app_purchase/in_app_purchase_android/pubspec.yaml

SynSzakala changed the title ~~Recreating PR from flutter/plugins~~ [in_app_purchase_android] Implement BillingClient connection management and introduce BillingClientManager Feb 26, 2023

SynSzakala changed the title ~~[in_app_purchase_android] Implement BillingClient connection management and introduce BillingClientManager~~ [in_app_purchase_android] Implement BillingClient connection management and introduce BillingClientManager Feb 26, 2023

bparrishMines requested changes Mar 1, 2023

View reviewed changes

jakub.walusiak and others added 2 commits March 6, 2023 10:07

[in_app_purchases_android_platform] Changed method names in BillingCl…

d41bc53

…ientManager

Merge branch 'main' into fix_in_app_purchase_android_billing_client_r…

e3b1e5d

…econnect

SynSzakala requested review from bparrishMines and removed request for gmackall March 6, 2023 09:09

[in_app_purchases_android_platform] Changed method names in BillingCl…

3730512

…ientManager

bparrishMines reviewed Mar 9, 2023

View reviewed changes

jakub.walusiak added 3 commits March 11, 2023 19:13

Merge remote-tracking branch 'upstream/main' into fix_in_app_purchase…

52b5a54

…_android_billing_client_reconnect # Conflicts: # packages/in_app_purchase/in_app_purchase_android/CHANGELOG.md # packages/in_app_purchase/in_app_purchase_android/pubspec.yaml

[in_app_purchases_android_platform] Reformat files, fix comments.

7ad117a

[in_app_purchases_android_platform] Remove unnecessary refactor.

a525c4e

SynSzakala requested a review from bparrishMines March 11, 2023 18:27

bparrishMines approved these changes Mar 15, 2023

View reviewed changes

jakub.walusiak added 2 commits March 17, 2023 10:10

[in_app_purchases_android_platform] Update CHANGELOG.md

96b6eef

[in_app_purchases_android_platform] Update site-shared

6257582

SynSzakala requested a review from bparrishMines March 17, 2023 09:13

bparrishMines approved these changes Mar 23, 2023

View reviewed changes

Merge branch 'main' into fix_in_app_purchase_android_billing_client_r…

e321015

…econnect

bparrishMines requested review from stuartmorgan-g and tarrinneal April 5, 2023 17:26

tarrinneal approved these changes Apr 5, 2023

View reviewed changes

bparrishMines added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 10, 2023

auto-submit bot merged commit b0a58d5 into flutter:main Apr 10, 2023

bparrishMines mentioned this pull request Apr 10, 2023

[in_app_purchase_android] Fix mockMethodCallHandler lint #3674

Merged

11 tasks

engine-flutter-autoroll mentioned this pull request Apr 10, 2023

Roll Packages from 75ede70ca5f0 to b0a58d551cbf (7 revisions) flutter/flutter#124516

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 10, 2023

b0a58d5 [in_app_purchase_android] Implement BillingClient connectio…

bce7c57

…n management and introduce `BillingClientManager` (flutter/packages#3303)

engine-flutter-autoroll mentioned this pull request Apr 10, 2023

Roll Packages from 75ede70ca5f0 to a2f396cc91db (8 revisions) flutter/flutter#124519

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 10, 2023

b0a58d5 [in_app_purchase_android] Implement BillingClient connectio…

cbc9ee5

…n management and introduce `BillingClientManager` (flutter/packages#3303)

	/// - Further connection attempts will not be made;
	/// - Further connection attempts will not be made.

	/// - [purchasesUpdatedStream] will be closed;
	/// - [purchasesUpdatedStream] will be closed.

		@@ -1,3 +1,8 @@
		## 0.2.5

		* Fixes the management of `BillingClient` connection.

[in_app_purchase_android] Implement BillingClient connection management and introduce BillingClientManager #3303

[in_app_purchase_android] Implement BillingClient connection management and introduce BillingClientManager #3303

Uh oh!

Conversation

SynSzakala commented Feb 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

SynSzakala commented Feb 26, 2023

Uh oh!

bparrishMines left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparrishMines left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparrishMines left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SynSzakala commented Mar 17, 2023

Uh oh!

bparrishMines left a comment

Choose a reason for hiding this comment

Uh oh!

tarrinneal left a comment

Choose a reason for hiding this comment

Uh oh!

SynSzakala commented Apr 6, 2023

Uh oh!

bparrishMines commented Apr 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[in_app_purchase_android] Implement `BillingClient` connection management and introduce `BillingClientManager` #3303

[in_app_purchase_android] Implement `BillingClient` connection management and introduce `BillingClientManager` #3303

SynSzakala commented Feb 26, 2023 •

edited

Loading