diff --git a/packages/common_client/lib/launchdarkly_common_client.dart b/packages/common_client/lib/launchdarkly_common_client.dart index 50a01e3e..5537658e 100644 --- a/packages/common_client/lib/launchdarkly_common_client.dart +++ b/packages/common_client/lib/launchdarkly_common_client.dart @@ -118,6 +118,7 @@ export 'src/plugins/operations.dart' safeGetHooks, safeGetPluginHooks, safeGetPluginName, + safeRegisterPlugin, safeRegisterPlugins; export 'src/config/defaults/credential_type.dart' show CredentialType; diff --git a/packages/common_client/lib/src/plugins/operations.dart b/packages/common_client/lib/src/plugins/operations.dart index 45f32d90..118908f3 100644 --- a/packages/common_client/lib/src/plugins/operations.dart +++ b/packages/common_client/lib/src/plugins/operations.dart @@ -38,17 +38,58 @@ List? safeGetHooks( .toList(); } +/// Registers a single [plugin] with [client] and then activates the hooks it +/// contributes. +/// +/// This is the single plugin case of [safeRegisterPlugins], and behaves the same +/// way. +void safeRegisterPlugin( + TClient client, + PluginEnvironmentMetadata metadata, + PluginBase plugin, + void Function(Hook hook) addHook, + LDLogger logger) { + safeRegisterPlugins(client, metadata, [plugin], addHook, logger); +} + +/// Registers each of [plugins] with [client], then activates the hooks +/// contributed by those that registered successfully. +/// +/// The hooks are activated as the last step, once every plugin has registered, +/// so that no plugin's hooks observe any plugin's [PluginBase.register] call, +/// and a plugin that failed either step contributes none. Exceptions are logged +/// rather than rethrown, so one failing plugin does not stop the others being +/// registered. void safeRegisterPlugins( TClient client, PluginEnvironmentMetadata metadata, List>? plugins, + void Function(Hook hook) addHook, LDLogger logger) { - plugins?.forEach((plugin) { + if (plugins == null) { + return; + } + + final hooksToActivate = []; + + for (final plugin in plugins) { + final hooks = safeGetPluginHooks(plugin, logger); + if (hooks == null) { + continue; + } + try { plugin.register(client, metadata); } catch (err) { logger.warn( 'Exception thrown when registering plugin ${safeGetPluginName(plugin, logger)}'); + continue; } - }); + + hooksToActivate.addAll(hooks); + } + + for (final hook in hooksToActivate) { + addHook(hook); + } } diff --git a/packages/common_client/test/plugins/operations_test.dart b/packages/common_client/test/plugins/operations_test.dart index 7d5e2798..ca2ab630 100644 --- a/packages/common_client/test/plugins/operations_test.dart +++ b/packages/common_client/test/plugins/operations_test.dart @@ -292,9 +292,11 @@ void main() { group('safeRegisterPlugins', () { late PluginEnvironmentMetadata testEnvironmentMetadata; late dynamic testClient; + late List activatedHooks; setUp(() { testClient = 'test-client'; + activatedHooks = []; testEnvironmentMetadata = PluginEnvironmentMetadata( sdk: PluginSdkMetadata( name: 'test-sdk', @@ -308,20 +310,22 @@ void main() { }); test('does nothing when plugins list is null', () { - safeRegisterPlugins(testClient, testEnvironmentMetadata, null, logger); + safeRegisterPlugins( + testClient, testEnvironmentMetadata, null, activatedHooks.add, logger); // No exceptions should be thrown, function should complete silently }); test('does nothing when plugins list is empty', () { - safeRegisterPlugins(testClient, testEnvironmentMetadata, [], logger); + safeRegisterPlugins( + testClient, testEnvironmentMetadata, [], activatedHooks.add, logger); // No exceptions should be thrown, function should complete silently }); test('registers single plugin successfully', () { final plugin = TestPlugin('test-plugin', []); - safeRegisterPlugins( - testClient, testEnvironmentMetadata, [plugin], logger); + safeRegisterPlugins(testClient, testEnvironmentMetadata, [plugin], + activatedHooks.add, logger); expect(plugin.registerCallCount, equals(1)); expect(plugin.lastClientReceived, same(testClient)); @@ -335,7 +339,7 @@ void main() { final plugin3 = TestPlugin('plugin-3', []); safeRegisterPlugins(testClient, testEnvironmentMetadata, - [plugin1, plugin2, plugin3], logger); + [plugin1, plugin2, plugin3], activatedHooks.add, logger); expect(plugin1.registerCallCount, equals(1)); expect(plugin1.lastClientReceived, same(testClient)); @@ -353,6 +357,32 @@ void main() { same(testEnvironmentMetadata)); }); + test('activates hooks of registered plugins in order', () { + final hook1 = TestHook('hook-1'); + final hook2 = TestHook('hook-2'); + final hook3 = TestHook('hook-3'); + final plugin1 = TestPlugin('plugin-1', [hook1, hook2]); + final plugin2 = TestPlugin('plugin-2', [hook3]); + + safeRegisterPlugins(testClient, testEnvironmentMetadata, + [plugin1, plugin2], activatedHooks.add, logger); + + expect(activatedHooks, equals([hook1, hook2, hook3])); + }); + + test('does not activate hooks of a plugin that fails to register', () { + final goodHook = TestHook('good-hook'); + final badHook = TestHook('bad-hook'); + final goodPlugin = TestPlugin('good-plugin', [goodHook]); + final badPlugin = TestPlugin('bad-plugin', [badHook], + shouldThrowOnRegister: true); + + safeRegisterPlugins(testClient, testEnvironmentMetadata, + [goodPlugin, badPlugin], activatedHooks.add, logger); + + expect(activatedHooks, equals([goodHook])); + }); + test('handles exception from single plugin registration and logs warning', () { final plugin1 = TestPlugin('plugin-1', []); @@ -360,7 +390,7 @@ void main() { final plugin3 = TestPlugin('plugin-3', []); safeRegisterPlugins(testClient, testEnvironmentMetadata, - [plugin1, plugin2, plugin3], logger); + [plugin1, plugin2, plugin3], activatedHooks.add, logger); // First and third plugins should be registered successfully expect(plugin1.registerCallCount, equals(1)); @@ -385,7 +415,7 @@ void main() { final plugin4 = TestPlugin('plugin-4', []); safeRegisterPlugins(testClient, testEnvironmentMetadata, - [plugin1, plugin2, plugin3, plugin4], logger); + [plugin1, plugin2, plugin3, plugin4], activatedHooks.add, logger); // First and fourth plugins should be registered successfully expect(plugin1.registerCallCount, equals(1)); @@ -414,8 +444,8 @@ void main() { final plugin1 = TestPlugin('plugin-1', [], shouldThrowOnRegister: true); final plugin2 = TestPlugin('plugin-2', [], shouldThrowOnRegister: true); - safeRegisterPlugins( - testClient, testEnvironmentMetadata, [plugin1, plugin2], logger); + safeRegisterPlugins(testClient, testEnvironmentMetadata, + [plugin1, plugin2], activatedHooks.add, logger); // All plugins should have attempted registration but failed expect(plugin1.registerCallCount, equals(1)); @@ -440,7 +470,8 @@ void main() { final plugin5 = TestPlugin('plugin-5', []); safeRegisterPlugins(testClient, testEnvironmentMetadata, - [plugin1, plugin2, plugin3, plugin4, plugin5], logger); + [plugin1, plugin2, plugin3, plugin4, plugin5], activatedHooks.add, + logger); // Successful plugins should be registered expect(plugin1.registerCallCount, equals(1)); @@ -478,8 +509,8 @@ void main() { ), ); - safeRegisterPlugins( - customClient, customEnvironmentMetadata, [plugin1, plugin2], logger); + safeRegisterPlugins(customClient, customEnvironmentMetadata, + [plugin1, plugin2], activatedHooks.add, logger); expect(plugin1.registerCallCount, equals(1)); expect(plugin1.lastClientReceived, same(customClient)); @@ -501,7 +532,7 @@ void main() { final plugin3 = TestPlugin('plugin-3', []); safeRegisterPlugins(testClient, testEnvironmentMetadata, - [plugin1, plugin2, plugin3], logger); + [plugin1, plugin2, plugin3], activatedHooks.add, logger); // All plugins should have attempted registration expect(plugin1.registerCallCount, equals(1)); diff --git a/packages/flutter_client_sdk/lib/src/ld_client.dart b/packages/flutter_client_sdk/lib/src/ld_client.dart index df2de428..94de6672 100644 --- a/packages/flutter_client_sdk/lib/src/ld_client.dart +++ b/packages/flutter_client_sdk/lib/src/ld_client.dart @@ -71,12 +71,11 @@ interface class LDClient { platformEnvReporter: PlatformEnvReporter(), autoEnvAttributes: config.autoEnvAttributes == AutoEnvAttributes.enabled); - final pluginHooks = safeGetHooks(config.plugins, config.logger); - final combined = combineHooks(config.hooks, pluginHooks); - + // Only the configuration's own hooks. A plugin's hooks are added once that plugin has + // registered, below, which is still before `start` opens the first identify series. _client = LDCommonClient(config, platformImplementation, context, DiagnosticSdkData(name: sdkName, version: sdkVersion), - hooks: combined); + hooks: config.hooks); final stateDetector = FlutterStateDetector(); // Under the FDv2 data system the connection mode is governed by the // data system configuration, not the FDv1 data source options: the @@ -122,8 +121,8 @@ interface class LDClient { credential: PluginCredentialInfo( type: _client.credentialType, value: config.sdkCredential)); - safeRegisterPlugins( - this, _pluginEnvironmentMetadata, config.plugins, config.logger); + safeRegisterPlugins(this, _pluginEnvironmentMetadata, config.plugins, + addHook, config.logger); } /// Initialize the SDK. @@ -403,21 +402,14 @@ interface class LDClient { /// Registers a plugin with this SDK instance after the client has been /// constructed. /// - /// Bundled hooks from the plugin are added before [Plugin.register] is - /// invoked. If reading [Plugin.hooks] throws, the plugin is not registered. - /// If [Plugin.register] throws, the error is logged and not rethrown. + /// The plugin's bundled hooks are added only once [Plugin.register] has + /// returned, so they do not observe what `register` itself does, and a plugin + /// that fails either step contributes no hooks. The plugins in + /// [LDConfig.plugins] are registered by this same path, so a plugin behaves + /// the same however it was registered. Errors are logged, not rethrown. void registerPlugin(Plugin plugin) { - final hooks = safeGetPluginHooks(plugin, _client.logger); - if (hooks == null) { - return; - } - - for (final hook in hooks) { - addHook(hook); - } - - safeRegisterPlugins( - this, _pluginEnvironmentMetadata, [plugin], _client.logger); + safeRegisterPlugin( + this, _pluginEnvironmentMetadata, plugin, addHook, _client.logger); } } diff --git a/packages/flutter_client_sdk/test/ld_client_plugin_test.dart b/packages/flutter_client_sdk/test/ld_client_plugin_test.dart index 9b74a40a..cd6d7eaf 100644 --- a/packages/flutter_client_sdk/test/ld_client_plugin_test.dart +++ b/packages/flutter_client_sdk/test/ld_client_plugin_test.dart @@ -226,6 +226,27 @@ void main() { client.close(); }); + test('plugin hooks do not observe another plugin register', () { + final firstHook = TestHook('first-plugin-hook'); + final firstPlugin = TestPlugin('first-plugin', [firstHook]); + // Registers after the first plugin, and evaluates a flag while doing so. + final secondPlugin = + EvaluationOnRegisterPlugin(TestHook('second-plugin-hook')); + + final client = createTestClient(plugins: [firstPlugin, secondPlugin]); + + // Hooks are activated only once every plugin has registered, so the first plugin's hook did + // not observe the evaluation the second plugin made while registering. + expect(secondPlugin.registerCallCount, equals(1)); + expect(firstHook.callLog, isEmpty); + + client.boolVariation('test-flag', false); + expect(firstHook.callLog.any((call) => call.startsWith('beforeEvaluation')), + isTrue); + + client.close(); + }); + test('registers hooks from single plugin', () { final hook1 = TestHook('plugin-hook-1'); final hook2 = TestHook('plugin-hook-2'); @@ -459,18 +480,22 @@ void main() { client.close(); }); - test('adds hooks before plugin register is invoked', () { + test('adds hooks only after plugin register has returned', () { final hook = TestHook('runtime-order-hook'); final plugin = EvaluationOnRegisterPlugin(hook); final client = createTestClient(); client.registerPlugin(plugin); + // The hooks go live only once register has returned, so the evaluation that register made did + // not reach them. expect(plugin.registerCallCount, equals(1)); + expect(hook.callLog, isEmpty); + + // They do run for evaluations made once registration has completed. + client.boolVariation('test-flag', false); expect(hook.callLog.any((call) => call.startsWith('beforeEvaluation')), isTrue); - expect(hook.callLog.any((call) => call.startsWith('afterEvaluation')), - isTrue); client.close(); });