From 53a7474351b2f67f9256af34b3a6d3f07a7908dc Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 7 Jun 2023 13:16:24 -0400 Subject: [PATCH 1/2] [shared_preferences] Fix initialization race 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. Fixes https://github.com/flutter/flutter/issues/42407 --- .../shared_preferences/CHANGELOG.md | 4 +++- .../lib/shared_preferences.dart | 2 +- .../shared_preferences/pubspec.yaml | 2 +- .../test/shared_preferences_test.dart | 19 +++++++++++++++++++ 4 files changed, 24 insertions(+), 3 deletions(-) diff --git a/packages/shared_preferences/shared_preferences/CHANGELOG.md b/packages/shared_preferences/shared_preferences/CHANGELOG.md index 3ee0cabefd7..af254c9aa78 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 b7690e06756..26e489a6e27 100644 --- a/packages/shared_preferences/shared_preferences/lib/shared_preferences.dart +++ b/packages/shared_preferences/shared_preferences/lib/shared_preferences.dart @@ -62,6 +62,7 @@ class SharedPreferences { if (_completer == null) { final Completer completer = Completer(); + _completer = completer; try { final Map preferencesMap = await _getSharedPreferencesMap(); @@ -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 02335fe2e7a..2771b99eea6 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 06c853dce9b..1148c868e8e 100755 --- a/packages/shared_preferences/shared_preferences/test/shared_preferences_test.dart +++ b/packages/shared_preferences/shared_preferences/test/shared_preferences_test.dart @@ -214,6 +214,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 +377,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() { From 7acd9d5b153d9e2cb5c74015e5a15c513117b39f Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 7 Jun 2023 14:19:52 -0400 Subject: [PATCH 2/2] Remove extra test group, make catch handle Errors --- .../lib/shared_preferences.dart | 2 +- .../test/shared_preferences_test.dart | 339 +++++++++--------- 2 files changed, 169 insertions(+), 172 deletions(-) diff --git a/packages/shared_preferences/shared_preferences/lib/shared_preferences.dart b/packages/shared_preferences/shared_preferences/lib/shared_preferences.dart index 26e489a6e27..c116c9d4c3c 100644 --- a/packages/shared_preferences/shared_preferences/lib/shared_preferences.dart +++ b/packages/shared_preferences/shared_preferences/lib/shared_preferences.dart @@ -67,7 +67,7 @@ class SharedPreferences { 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); 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 1148c868e8e..805d5ed6111 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 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; - - setUp(() async { - store = FakeSharedPreferencesStore(testValues); - SharedPreferencesStorePlatform.instance = store; - preferences = await SharedPreferences.getInstance(); - store.log.clear(); - }); + 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, + }; + + late FakeSharedPreferencesStore store; + late SharedPreferences preferences; + + 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( + 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('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('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(); - - 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([])); - }); + List.filled( + 1, + isMethodCall( + 'remove', + arguments: 'flutter.$key', + ), + growable: true, + )); + }); - 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'; - test('containsKey', () async { - const String key = 'testKey'; + expect(false, preferences.containsKey(key)); - expect(false, preferences.containsKey(key)); + await preferences.setString(key, 'test'); + expect(true, 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('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); - test('reloading', () async { - await preferences.setString('String', testString); - expect(preferences.getString('String'), testString); + SharedPreferences.setMockInitialValues(testValues2.cast()); + expect(preferences.getString('String'), testString); - SharedPreferences.setMockInitialValues( - testValues2.cast()); - expect(preferences.getString('String'), testString); + await preferences.reload(); + expect(preferences.getString('String'), testString2); + }); - 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('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']); + }); + + group('mocking', () { + const String key = 'dummy'; + const String prefixedKey = 'flutter.$key'; - test('string list type is dynamic (usually from method channel)', () async { - SharedPreferences.setMockInitialValues({ - 'dynamic_list': ['1', '2'] - }); + test('test 1', () async { + SharedPreferences.setMockInitialValues( + {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 {