From d935cb0d2f38ecdc3bf79f1b423b6ed33d66f2e5 Mon Sep 17 00:00:00 2001 From: stuartmorgan Date: Wed, 7 Jun 2023 22:34:57 -0400 Subject: [PATCH] [shared_preferences] Fix initialization race (#4159) During the NNBD transition, the structure of the completer logic in the initialization flow was incorrectly changed to set the field after an `await` instead of immediately. Also updates the error handling to handle `Error` the same way it currently handles `Exception`, which this change surfaced. Fixes https://github.com/flutter/flutter/issues/42407 --- .../shared_preferences/CHANGELOG.md | 4 +- .../lib/shared_preferences.dart | 4 +- .../shared_preferences/pubspec.yaml | 2 +- .../test/shared_preferences_test.dart | 364 +++++++++--------- 4 files changed, 196 insertions(+), 178 deletions(-) diff --git a/packages/shared_preferences/shared_preferences/CHANGELOG.md b/packages/shared_preferences/shared_preferences/CHANGELOG.md index 3ee0cabefd..af254c9aa7 100644 --- a/packages/shared_preferences/shared_preferences/CHANGELOG.md +++ b/packages/shared_preferences/shared_preferences/CHANGELOG.md @@ -1,5 +1,7 @@ -## NEXT +## 2.1.2 +* Fixes singleton initialization race condition introduced during NNBD + transition. * Updates minimum supported macOS version to 10.14. * Updates minimum supported SDK version to Flutter 3.3/Dart 2.18. diff --git a/packages/shared_preferences/shared_preferences/lib/shared_preferences.dart b/packages/shared_preferences/shared_preferences/lib/shared_preferences.dart index b7690e0675..c116c9d4c3 100644 --- a/packages/shared_preferences/shared_preferences/lib/shared_preferences.dart +++ b/packages/shared_preferences/shared_preferences/lib/shared_preferences.dart @@ -62,11 +62,12 @@ class SharedPreferences { if (_completer == null) { final Completer completer = Completer(); + _completer = completer; try { final Map preferencesMap = await _getSharedPreferencesMap(); completer.complete(SharedPreferences._(preferencesMap)); - } on Exception catch (e) { + } catch (e) { // If there's an error, explicitly return the future with an error. // then set the completer to null so we can retry. completer.completeError(e); @@ -74,7 +75,6 @@ class SharedPreferences { _completer = null; return sharedPrefsFuture; } - _completer = completer; } return _completer!.future; } diff --git a/packages/shared_preferences/shared_preferences/pubspec.yaml b/packages/shared_preferences/shared_preferences/pubspec.yaml index 02335fe2e7..2771b99eea 100644 --- a/packages/shared_preferences/shared_preferences/pubspec.yaml +++ b/packages/shared_preferences/shared_preferences/pubspec.yaml @@ -3,7 +3,7 @@ description: Flutter plugin for reading and writing simple key-value pairs. Wraps NSUserDefaults on iOS and SharedPreferences on Android. repository: https://github.com/flutter/packages/tree/main/packages/shared_preferences/shared_preferences issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+shared_preferences%22 -version: 2.1.1 +version: 2.1.2 environment: sdk: ">=2.18.0 <4.0.0" diff --git a/packages/shared_preferences/shared_preferences/test/shared_preferences_test.dart b/packages/shared_preferences/shared_preferences/test/shared_preferences_test.dart index 06c853dce9..805d5ed611 100755 --- a/packages/shared_preferences/shared_preferences/test/shared_preferences_test.dart +++ b/packages/shared_preferences/shared_preferences/test/shared_preferences_test.dart @@ -10,199 +10,196 @@ import 'package:shared_preferences_platform_interface/shared_preferences_platfor void main() { TestWidgetsFlutterBinding.ensureInitialized(); - group('SharedPreferences', () { - const String testString = 'hello world'; - const bool testBool = true; - const int testInt = 42; - const double testDouble = 3.14159; - const List testList = ['foo', 'bar']; - const Map testValues = { - 'flutter.String': testString, - 'flutter.bool': testBool, - 'flutter.int': testInt, - 'flutter.double': testDouble, - 'flutter.List': testList, - }; + const String testString = 'hello world'; + const bool testBool = true; + const int testInt = 42; + const double testDouble = 3.14159; + const List testList = ['foo', 'bar']; + const Map testValues = { + 'flutter.String': testString, + 'flutter.bool': testBool, + 'flutter.int': testInt, + 'flutter.double': testDouble, + 'flutter.List': testList, + }; - const String testString2 = 'goodbye world'; - const bool testBool2 = false; - const int testInt2 = 1337; - const double testDouble2 = 2.71828; - const List testList2 = ['baz', 'quox']; - const Map testValues2 = { - 'flutter.String': testString2, - 'flutter.bool': testBool2, - 'flutter.int': testInt2, - 'flutter.double': testDouble2, - 'flutter.List': testList2, - }; + const String testString2 = 'goodbye world'; + const bool testBool2 = false; + const int testInt2 = 1337; + const double testDouble2 = 2.71828; + const List testList2 = ['baz', 'quox']; + const Map testValues2 = { + 'flutter.String': testString2, + 'flutter.bool': testBool2, + 'flutter.int': testInt2, + 'flutter.double': testDouble2, + 'flutter.List': testList2, + }; - late FakeSharedPreferencesStore store; - late SharedPreferences preferences; + late FakeSharedPreferencesStore store; + late SharedPreferences preferences; - setUp(() async { - store = FakeSharedPreferencesStore(testValues); - SharedPreferencesStorePlatform.instance = store; - preferences = await SharedPreferences.getInstance(); - store.log.clear(); - }); + setUp(() async { + store = FakeSharedPreferencesStore(testValues); + SharedPreferencesStorePlatform.instance = store; + preferences = await SharedPreferences.getInstance(); + store.log.clear(); + }); - test('reading', () async { - expect(preferences.get('String'), testString); - expect(preferences.get('bool'), testBool); - expect(preferences.get('int'), testInt); - expect(preferences.get('double'), testDouble); - expect(preferences.get('List'), testList); - expect(preferences.getString('String'), testString); - expect(preferences.getBool('bool'), testBool); - expect(preferences.getInt('int'), testInt); - expect(preferences.getDouble('double'), testDouble); - expect(preferences.getStringList('List'), testList); - expect(store.log, []); - }); + test('reading', () async { + expect(preferences.get('String'), testString); + expect(preferences.get('bool'), testBool); + expect(preferences.get('int'), testInt); + expect(preferences.get('double'), testDouble); + expect(preferences.get('List'), testList); + expect(preferences.getString('String'), testString); + expect(preferences.getBool('bool'), testBool); + expect(preferences.getInt('int'), testInt); + expect(preferences.getDouble('double'), testDouble); + expect(preferences.getStringList('List'), testList); + expect(store.log, []); + }); - test('writing', () async { - await Future.wait(>[ - preferences.setString('String', testString2), - preferences.setBool('bool', testBool2), - preferences.setInt('int', testInt2), - preferences.setDouble('double', testDouble2), - preferences.setStringList('List', testList2) - ]); - expect( + test('writing', () async { + await Future.wait(>[ + preferences.setString('String', testString2), + preferences.setBool('bool', testBool2), + preferences.setInt('int', testInt2), + preferences.setDouble('double', testDouble2), + preferences.setStringList('List', testList2) + ]); + expect( + store.log, + [ + isMethodCall('setValue', arguments: [ + 'String', + 'flutter.String', + testString2, + ]), + isMethodCall('setValue', arguments: [ + 'Bool', + 'flutter.bool', + testBool2, + ]), + isMethodCall('setValue', arguments: [ + 'Int', + 'flutter.int', + testInt2, + ]), + isMethodCall('setValue', arguments: [ + 'Double', + 'flutter.double', + testDouble2, + ]), + isMethodCall('setValue', arguments: [ + 'StringList', + 'flutter.List', + testList2, + ]), + ], + ); + store.log.clear(); + + expect(preferences.getString('String'), testString2); + expect(preferences.getBool('bool'), testBool2); + expect(preferences.getInt('int'), testInt2); + expect(preferences.getDouble('double'), testDouble2); + expect(preferences.getStringList('List'), testList2); + expect(store.log, equals([])); + }); + + test('removing', () async { + const String key = 'testKey'; + await preferences.remove(key); + expect( store.log, - [ - isMethodCall('setValue', arguments: [ - 'String', - 'flutter.String', - testString2, - ]), - isMethodCall('setValue', arguments: [ - 'Bool', - 'flutter.bool', - testBool2, - ]), - isMethodCall('setValue', arguments: [ - 'Int', - 'flutter.int', - testInt2, - ]), - isMethodCall('setValue', arguments: [ - 'Double', - 'flutter.double', - testDouble2, - ]), - isMethodCall('setValue', arguments: [ - 'StringList', - 'flutter.List', - testList2, - ]), - ], - ); - store.log.clear(); + List.filled( + 1, + isMethodCall( + 'remove', + arguments: 'flutter.$key', + ), + growable: true, + )); + }); - expect(preferences.getString('String'), testString2); - expect(preferences.getBool('bool'), testBool2); - expect(preferences.getInt('int'), testInt2); - expect(preferences.getDouble('double'), testDouble2); - expect(preferences.getStringList('List'), testList2); - expect(store.log, equals([])); + test('containsKey', () async { + const String key = 'testKey'; + + expect(false, preferences.containsKey(key)); + + await preferences.setString(key, 'test'); + expect(true, preferences.containsKey(key)); + }); + + test('clearing', () async { + await preferences.clear(); + expect(preferences.getString('String'), null); + expect(preferences.getBool('bool'), null); + expect(preferences.getInt('int'), null); + expect(preferences.getDouble('double'), null); + expect(preferences.getStringList('List'), null); + expect(store.log, [isMethodCall('clear', arguments: null)]); + }); + + test('reloading', () async { + await preferences.setString('String', testString); + expect(preferences.getString('String'), testString); + + SharedPreferences.setMockInitialValues(testValues2.cast()); + expect(preferences.getString('String'), testString); + + await preferences.reload(); + expect(preferences.getString('String'), testString2); + }); + + test('back to back calls should return same instance.', () async { + final Future first = SharedPreferences.getInstance(); + final Future second = SharedPreferences.getInstance(); + expect(await first, await second); + }); + + test('string list type is dynamic (usually from method channel)', () async { + SharedPreferences.setMockInitialValues({ + 'dynamic_list': ['1', '2'] }); + final SharedPreferences prefs = await SharedPreferences.getInstance(); + final List? value = prefs.getStringList('dynamic_list'); + expect(value, ['1', '2']); + }); - test('removing', () async { - const String key = 'testKey'; - await preferences.remove(key); - expect( - store.log, - List.filled( - 1, - isMethodCall( - 'remove', - arguments: 'flutter.$key', - ), - growable: true, - )); - }); - - test('containsKey', () async { - const String key = 'testKey'; - - expect(false, preferences.containsKey(key)); - - await preferences.setString(key, 'test'); - expect(true, preferences.containsKey(key)); - }); - - test('clearing', () async { - await preferences.clear(); - expect(preferences.getString('String'), null); - expect(preferences.getBool('bool'), null); - expect(preferences.getInt('int'), null); - expect(preferences.getDouble('double'), null); - expect(preferences.getStringList('List'), null); - expect(store.log, [isMethodCall('clear', arguments: null)]); - }); - - test('reloading', () async { - await preferences.setString('String', testString); - expect(preferences.getString('String'), testString); + group('mocking', () { + const String key = 'dummy'; + const String prefixedKey = 'flutter.$key'; + test('test 1', () async { SharedPreferences.setMockInitialValues( - testValues2.cast()); - expect(preferences.getString('String'), testString); - - await preferences.reload(); - expect(preferences.getString('String'), testString2); - }); - - test('back to back calls should return same instance.', () async { - final Future first = SharedPreferences.getInstance(); - final Future second = SharedPreferences.getInstance(); - expect(await first, await second); - }); - - test('string list type is dynamic (usually from method channel)', () async { - SharedPreferences.setMockInitialValues({ - 'dynamic_list': ['1', '2'] - }); + {prefixedKey: 'my string'}); final SharedPreferences prefs = await SharedPreferences.getInstance(); - final List? value = prefs.getStringList('dynamic_list'); - expect(value, ['1', '2']); + final String? value = prefs.getString(key); + expect(value, 'my string'); }); - group('mocking', () { - const String key = 'dummy'; - const String prefixedKey = 'flutter.$key'; - - test('test 1', () async { - SharedPreferences.setMockInitialValues( - {prefixedKey: 'my string'}); - final SharedPreferences prefs = await SharedPreferences.getInstance(); - final String? value = prefs.getString(key); - expect(value, 'my string'); - }); - - test('test 2', () async { - SharedPreferences.setMockInitialValues( - {prefixedKey: 'my other string'}); - final SharedPreferences prefs = await SharedPreferences.getInstance(); - final String? value = prefs.getString(key); - expect(value, 'my other string'); - }); + test('test 2', () async { + SharedPreferences.setMockInitialValues( + {prefixedKey: 'my other string'}); + final SharedPreferences prefs = await SharedPreferences.getInstance(); + final String? value = prefs.getString(key); + expect(value, 'my other string'); }); + }); - test('writing copy of strings list', () async { - final List myList = []; - await preferences.setStringList('myList', myList); - myList.add('foobar'); + test('writing copy of strings list', () async { + final List myList = []; + await preferences.setStringList('myList', myList); + myList.add('foobar'); - final List cachedList = preferences.getStringList('myList')!; - expect(cachedList, []); + final List cachedList = preferences.getStringList('myList')!; + expect(cachedList, []); - cachedList.add('foobar2'); + cachedList.add('foobar2'); - expect(preferences.getStringList('myList'), []); - }); + expect(preferences.getStringList('myList'), []); }); test('calling mock initial values with non-prefixed keys succeeds', () async { @@ -214,6 +211,16 @@ void main() { expect(value, 'foo'); }); + test('getInstance always returns the same instance', () async { + SharedPreferencesStorePlatform.instance = SlowInitSharedPreferencesStore(); + + final Future firstFuture = + SharedPreferences.getInstance(); + final Future secondFuture = + SharedPreferences.getInstance(); + expect(identical(await firstFuture, await secondFuture), true); + }); + test('calling setPrefix after getInstance throws', () async { const String newPrefix = 'newPrefix'; @@ -367,6 +374,15 @@ class UnimplementedSharedPreferencesStore } } +class SlowInitSharedPreferencesStore + extends UnimplementedSharedPreferencesStore { + @override + Future> getAll() async { + await Future.delayed(const Duration(seconds: 1)); + return {}; + } +} + class ThrowingSharedPreferencesStore extends SharedPreferencesStorePlatform { @override Future clear() {