Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[firebase_remote_config] Remote config/double instance error #2061

Closed
wants to merge 11 commits into from
Closed

[firebase_remote_config] Remote config/double instance error #2061

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a68cbbd
Fix side-effects of instance and doubleInstance tests
IchordeDionysos Feb 26, 2020
29d4844
Fix Future already completed error
IchordeDionysos Feb 26, 2020
04b13a5
Additional check method channel was called for doubleInstance test
IchordeDionysos Feb 26, 2020
0184d20
Use single quote strings
IchordeDionysos Feb 26, 2020
467e6b8
Use preferred if check over try/catch
IchordeDionysos Feb 26, 2020
2bcd6f7
Remove double if statement
IchordeDionysos Feb 26, 2020
1c38ea4
Update version and CHANGELOG
IchordeDionysos Feb 26, 2020
5c5cd65
Merge branch 'master' into remote_config/double-instance-error
IchordeDionysos Feb 26, 2020
f494d5d
Update version
IchordeDionysos Feb 26, 2020
9b3b451
Merge remote-tracking branch 'origin/remote_config/double-instance-er…
IchordeDionysos Feb 26, 2020
126ce56
Merge branch 'master' into remote_config/double-instance-error
IchordeDionysos Mar 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/firebase_remote_config/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## 0.3.0+4

* Fix `Bad state: Future already completed` error when initially
calling `RemoteConfig.instance` multiple times in parallel.

## 0.3.0+3

* Replace deprecated `getFlutterEngine` call on Android.
Expand Down
14 changes: 10 additions & 4 deletions packages/firebase_remote_config/lib/src/remote_config.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,23 @@ class RemoteConfig extends ChangeNotifier {
RemoteConfigSettings _remoteConfigSettings;

DateTime get lastFetchTime => _lastFetchTime;

LastFetchStatus get lastFetchStatus => _lastFetchStatus;

RemoteConfigSettings get remoteConfigSettings => _remoteConfigSettings;

static Completer<RemoteConfig> _instanceCompleter = Completer<RemoteConfig>();
@visibleForTesting
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're generally trying to avoid @VisibleForTesting APIs since they clutter the public logs, though I don't see another solution at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree, I tried to find an alternative first before using it, but have to change it back somehow...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FirebaseExtended/invertase @collinjackson is there any other solution to be able to test for this edge case?

static Completer<RemoteConfig> instanceCompleter = Completer<RemoteConfig>();

/// Gets the instance of RemoteConfig for the default Firebase app.
static Future<RemoteConfig> get instance async {
if (!_instanceCompleter.isCompleted) {
_instanceCompleter.complete(await _getRemoteConfigInstance());
if (!instanceCompleter.isCompleted) {
final _remoteConfigInstance = await _getRemoteConfigInstance();
if (!instanceCompleter.isCompleted) {
instanceCompleter.complete(_remoteConfigInstance);
}
}
return _instanceCompleter.future;
return instanceCompleter.future;
}

static Future<RemoteConfig> _getRemoteConfigInstance() async {
Expand Down
18 changes: 14 additions & 4 deletions packages/firebase_remote_config/test/firebase_remote_config_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:async';

import 'package:firebase_remote_config/firebase_remote_config.dart';
import 'package:flutter/services.dart';
import 'package:flutter_test/flutter_test.dart';
Expand Down Expand Up @@ -40,6 +42,12 @@ void main() {
});
});

tearDown(() {
RemoteConfig.instanceCompleter = Completer<RemoteConfig>();
// empty log for next tests
log.removeWhere((_) => true);
});

test('instance', () async {
final RemoteConfig remoteConfig = await RemoteConfig.instance;
expect(
Expand All @@ -59,10 +67,12 @@ void main() {
RemoteConfig.instance,
RemoteConfig.instance,
];
Future.wait(futures).then((List<RemoteConfig> remoteConfigs) {
// Check that both returned Remote Config instances are the same.
expect(remoteConfigs[0], remoteConfigs[1]);
});
final List<RemoteConfig> remoteConfigs = await Future.wait(futures);
// Check that both returned Remote Config instances are the same.
expect(remoteConfigs[0], remoteConfigs[1]);
// Check that method RemoteConfig#instance was called at least once
expect(log.length, isPositive);
expect(log[0], isMethodCall('RemoteConfig#instance', arguments: null));
});
});

Expand Down