From 9e2bf0fe1be0406a6e5ce95b0dcfaef14c0280de Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Tue, 25 Aug 2026 15:10:46 -0700 Subject: [PATCH] fix: activate a plugin's hooks only after it has registered A plugin's hooks now go live once every plugin has registered, rather than before register is called. This keeps a plugin's own hooks from observing what its register does, and keeps one plugin's hooks from observing another plugin's register regardless of list order. Both the plugins configured on LDConfig and those passed to registerPlugin now go through safeRegisterPlugins, so the two paths cannot drift apart. Co-authored-by: Cursor --- .../lib/launchdarkly_common_client.dart | 1 + .../lib/src/plugins/operations.dart | 45 ++++++++++++++- .../test/plugins/operations_test.dart | 57 ++++++++++++++----- .../flutter_client_sdk/lib/src/ld_client.dart | 32 ++++------- .../test/ld_client_plugin_test.dart | 31 +++++++++- 5 files changed, 128 insertions(+), 38 deletions(-) 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(); });