From bec84c1dbe41963d74fcafb3e77bf366d2aad4f1 Mon Sep 17 00:00:00 2001 From: Mehmet Fidanboylu Date: Thu, 12 Sep 2019 10:51:05 -0700 Subject: [PATCH 1/4] [google_sign_in] Fix chained async methods in error handling zones --- packages/google_sign_in/CHANGELOG.md | 5 + .../google_sign_in/lib/google_sign_in.dart | 117 ++++++++---------- packages/google_sign_in/pubspec.yaml | 2 +- .../test/google_sign_in_test.dart | 29 ++++- 4 files changed, 87 insertions(+), 66 deletions(-) mode change 100755 => 100644 packages/google_sign_in/lib/google_sign_in.dart mode change 100755 => 100644 packages/google_sign_in/pubspec.yaml diff --git a/packages/google_sign_in/CHANGELOG.md b/packages/google_sign_in/CHANGELOG.md index 92bf3230e06c..8cb60fd47497 100644 --- a/packages/google_sign_in/CHANGELOG.md +++ b/packages/google_sign_in/CHANGELOG.md @@ -1,3 +1,8 @@ +## 4.0.8 + +* Get rid of MethodCompleter and serialize async actions using chained futures. + This prevents a bug when sign in methods are being used in error handling zones. + ## 4.0.7 * Switch from using `api` to `implementation` for dependency on `play-services-auth`, diff --git a/packages/google_sign_in/lib/google_sign_in.dart b/packages/google_sign_in/lib/google_sign_in.dart old mode 100755 new mode 100644 index ca6e6bbaf705..22e3b08f2bbc --- a/packages/google_sign_in/lib/google_sign_in.dart +++ b/packages/google_sign_in/lib/google_sign_in.dart @@ -230,50 +230,57 @@ class GoogleSignIn { } Future _ensureInitialized() { - if (_initialization == null) { - _initialization = channel.invokeMethod('init', { - 'signInOption': (signInOption ?? SignInOption.standard).toString(), - 'scopes': scopes ?? [], - 'hostedDomain': hostedDomain, - }) - ..catchError((dynamic _) { - // Invalidate initialization if it errored out. - _initialization = null; - }); - } - return _initialization; + return _initialization ??= + channel.invokeMethod('init', { + 'signInOption': (signInOption ?? SignInOption.standard).toString(), + 'scopes': scopes ?? [], + 'hostedDomain': hostedDomain, + }) + ..catchError((dynamic _) { + // Invalidate initialization if it errored out. + _initialization = null; + }); } - /// Keeps track of the most recently scheduled method call. - _MethodCompleter _lastMethodCompleter; + /// The most recently scheduled method call. + Future _lastMethodCall; + + /// Returns a [Future] that completes with a success after [future], whether + /// it completed with a value or an error. + Future _waitFor(Future future) { + var completer = Completer(); + future.whenComplete(completer.complete).catchError((_) { + // Ignore if previous call completed with an error. + }); + return completer.future; + } /// Adds call to [method] in a queue for execution. /// /// At most one in flight call is allowed to prevent concurrent (out of order) /// updates to [currentUser] and [onCurrentUserChanged]. - Future _addMethodCall(String method) { - if (_lastMethodCompleter == null) { - _lastMethodCompleter = _MethodCompleter(method) - ..complete(_callMethod(method)); - return _lastMethodCompleter.future; + Future _addMethodCall(String method) async { + Future response; + if (_lastMethodCall == null) { + response = _callMethod(method); + } else { + response = _lastMethodCall.then((_) { + // If after the last completed call currentUser is not null and requested + // method is a sign in method, re-use the same authenticated user + // instead of making extra call to the native side. + const List kSignInMethods = [ + 'signIn', + 'signInSilently' + ]; + if (kSignInMethods.contains(method) && _currentUser != null) { + return _currentUser; + } else { + return _callMethod(method); + } + }); } - - final _MethodCompleter completer = _MethodCompleter(method); - _lastMethodCompleter.future.whenComplete(() { - // If after the last completed call currentUser is not null and requested - // method is a sign in method, re-use the same authenticated user - // instead of making extra call to the native side. - const List kSignInMethods = ['signIn', 'signInSilently']; - if (kSignInMethods.contains(method) && _currentUser != null) { - completer.complete(_currentUser); - } else { - completer.complete(_callMethod(method)); - } - }).catchError((dynamic _) { - // Ignore if previous call completed with an error. - }); - _lastMethodCompleter = completer; - return _lastMethodCompleter.future; + _lastMethodCall = _waitFor(response); + return response; } /// The currently signed in account, or null if the user is signed out. @@ -296,12 +303,17 @@ class GoogleSignIn { /// returned Future completes with [PlatformException] whose `code` can be /// either [kSignInRequiredError] (when there is no authenticated user) or /// [kSignInFailedError] (when an unknown error occurred). - Future signInSilently({bool suppressErrors = true}) { - final Future result = _addMethodCall('signInSilently'); - if (suppressErrors) { - return result.catchError((dynamic _) => null); + Future signInSilently( + {bool suppressErrors = true}) async { + try { + return await _addMethodCall('signInSilently'); + } catch (_) { + if (suppressErrors) { + return null; + } else { + rethrow; + } } - return result; } /// Returns a future that resolves to whether a user is currently signed in. @@ -334,26 +346,3 @@ class GoogleSignIn { /// authentication. Future disconnect() => _addMethodCall('disconnect'); } - -class _MethodCompleter { - _MethodCompleter(this.method); - - final String method; - final Completer _completer = - Completer(); - - Future complete(FutureOr value) async { - if (value is Future) { - try { - _completer.complete(await value); - } catch (e, stacktrace) { - _completer.completeError(e, stacktrace); - } - } else { - _completer.complete(value); - } - } - - bool get isCompleted => _completer.isCompleted; - Future get future => _completer.future; -} diff --git a/packages/google_sign_in/pubspec.yaml b/packages/google_sign_in/pubspec.yaml old mode 100755 new mode 100644 index 6ed758895bf7..246a6389e39c --- a/packages/google_sign_in/pubspec.yaml +++ b/packages/google_sign_in/pubspec.yaml @@ -3,7 +3,7 @@ description: Flutter plugin for Google Sign-In, a secure authentication system for signing in with a Google account on Android and iOS. author: Flutter Team homepage: https://github.com/flutter/plugins/tree/master/packages/google_sign_in -version: 4.0.7 +version: 4.0.8 flutter: plugin: diff --git a/packages/google_sign_in/test/google_sign_in_test.dart b/packages/google_sign_in/test/google_sign_in_test.dart index dab480a4cce4..2c89836aaf2e 100755 --- a/packages/google_sign_in/test/google_sign_in_test.dart +++ b/packages/google_sign_in/test/google_sign_in_test.dart @@ -45,7 +45,13 @@ void main() { responses = Map.from(kDefaultResponses); channel.setMockMethodCallHandler((MethodCall methodCall) { log.add(methodCall); - return Future.value(responses[methodCall.method]); + final response = responses[methodCall.method]; + if (response != null && + response is Map && + response.containsKey('error')) { + return Future.error(response['error']); + } + return Future.value(response); }); googleSignIn = GoogleSignIn(); log.clear(); @@ -142,6 +148,27 @@ void main() { ]); }); + test('signIn works even if a previous call throws error in other zone', + () async { + responses['signInSilently'] = {'error': 'Not a user'}; + await runZoned(() async { + expect(await googleSignIn.signInSilently(), isNull); + }, onError: (e, st) {}); + expect(await googleSignIn.signIn(), isNotNull); + expect( + log, + [ + isMethodCall('init', arguments: { + 'signInOption': 'SignInOption.standard', + 'scopes': [], + 'hostedDomain': null, + }), + isMethodCall('signInSilently', arguments: null), + isMethodCall('signIn', arguments: null), + ], + ); + }); + test('concurrent calls of the same method trigger sign in once', () async { final List> futures = >[ From 21ccfcb82bebd1434e4323053d96d81827a1f33a Mon Sep 17 00:00:00 2001 From: Mehmet Fidanboylu Date: Thu, 12 Sep 2019 11:06:27 -0700 Subject: [PATCH 2/4] Make analyzer happy. Swich to real Exception object. --- packages/google_sign_in/lib/google_sign_in.dart | 4 ++-- .../google_sign_in/test/google_sign_in_test.dart | 14 ++++++-------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/packages/google_sign_in/lib/google_sign_in.dart b/packages/google_sign_in/lib/google_sign_in.dart index 22e3b08f2bbc..64d323a2c6b0 100644 --- a/packages/google_sign_in/lib/google_sign_in.dart +++ b/packages/google_sign_in/lib/google_sign_in.dart @@ -248,8 +248,8 @@ class GoogleSignIn { /// Returns a [Future] that completes with a success after [future], whether /// it completed with a value or an error. Future _waitFor(Future future) { - var completer = Completer(); - future.whenComplete(completer.complete).catchError((_) { + final Completer completer = Completer(); + future.whenComplete(completer.complete).catchError((dynamic _) { // Ignore if previous call completed with an error. }); return completer.future; diff --git a/packages/google_sign_in/test/google_sign_in_test.dart b/packages/google_sign_in/test/google_sign_in_test.dart index 2c89836aaf2e..108edf9c892b 100755 --- a/packages/google_sign_in/test/google_sign_in_test.dart +++ b/packages/google_sign_in/test/google_sign_in_test.dart @@ -45,11 +45,9 @@ void main() { responses = Map.from(kDefaultResponses); channel.setMockMethodCallHandler((MethodCall methodCall) { log.add(methodCall); - final response = responses[methodCall.method]; - if (response != null && - response is Map && - response.containsKey('error')) { - return Future.error(response['error']); + final dynamic response = responses[methodCall.method]; + if (response != null && response is Exception) { + return Future.error('$response'); } return Future.value(response); }); @@ -150,10 +148,10 @@ void main() { test('signIn works even if a previous call throws error in other zone', () async { - responses['signInSilently'] = {'error': 'Not a user'}; + responses['signInSilently'] = Exception('Not a user'); await runZoned(() async { expect(await googleSignIn.signInSilently(), isNull); - }, onError: (e, st) {}); + }, onError: (dynamic e, dynamic st) {}); expect(await googleSignIn.signIn(), isNotNull); expect( log, @@ -197,7 +195,7 @@ void main() { }); test('can sign in after previously failed attempt', () async { - responses['signInSilently'] = {'error': 'Not a user'}; + responses['signInSilently'] = Exception('Not a user'); expect(await googleSignIn.signInSilently(), isNull); expect(await googleSignIn.signIn(), isNotNull); expect( From 650e69f66ab391122afef84f157fc9069c6e02bb Mon Sep 17 00:00:00 2001 From: Collin Jackson Date: Thu, 12 Sep 2019 13:56:10 -0700 Subject: [PATCH 3/4] Update CHANGELOG.md --- packages/google_sign_in/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/google_sign_in/CHANGELOG.md b/packages/google_sign_in/CHANGELOG.md index 8cb60fd47497..188167e53cae 100644 --- a/packages/google_sign_in/CHANGELOG.md +++ b/packages/google_sign_in/CHANGELOG.md @@ -1,6 +1,6 @@ ## 4.0.8 -* Get rid of MethodCompleter and serialize async actions using chained futures. +* Get rid of `MethodCompleter` and serialize async actions using chained futures. This prevents a bug when sign in methods are being used in error handling zones. ## 4.0.7 From 398abda7041deb087b522b98e3402f40d0ce8935 Mon Sep 17 00:00:00 2001 From: Mehmet Fidanboylu Date: Thu, 12 Sep 2019 14:11:41 -0700 Subject: [PATCH 4/4] Review comments --- packages/google_sign_in/lib/google_sign_in.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/google_sign_in/lib/google_sign_in.dart b/packages/google_sign_in/lib/google_sign_in.dart index 64d323a2c6b0..f1e1db21801e 100644 --- a/packages/google_sign_in/lib/google_sign_in.dart +++ b/packages/google_sign_in/lib/google_sign_in.dart @@ -247,7 +247,7 @@ class GoogleSignIn { /// Returns a [Future] that completes with a success after [future], whether /// it completed with a value or an error. - Future _waitFor(Future future) { + static Future _waitFor(Future future) { final Completer completer = Completer(); future.whenComplete(completer.complete).catchError((dynamic _) { // Ignore if previous call completed with an error. @@ -265,7 +265,7 @@ class GoogleSignIn { response = _callMethod(method); } else { response = _lastMethodCall.then((_) { - // If after the last completed call currentUser is not null and requested + // If after the last completed call `currentUser` is not `null` and requested // method is a sign in method, re-use the same authenticated user // instead of making extra call to the native side. const List kSignInMethods = [