fix: activate a plugin's hooks only after it has registered

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;

packages/common_client/lib/src/plugins/operations.dart

@@ -38,17 +38,58 @@ List<Hook>? safeGetHooks<TClient>(
       .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>(
+    TClient client,
+    PluginEnvironmentMetadata metadata,
+    PluginBase<TClient> 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>(
     TClient client,
     PluginEnvironmentMetadata metadata,
     List<PluginBase<TClient>>? plugins,
+    void Function(Hook hook) addHook,
     LDLogger logger) {
-  plugins?.forEach((plugin) {
+  if (plugins == null) {
+    return;
+  }
+
+  final hooksToActivate = <Hook>[];
+
+  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);
+  }
 }

packages/common_client/test/plugins/operations_test.dart

@@ -292,9 +292,11 @@ void main() {
   group('safeRegisterPlugins', () {
     late PluginEnvironmentMetadata testEnvironmentMetadata;
     late dynamic testClient;
+    late List<Hook> 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,14 +357,40 @@ 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', []);
       final plugin2 = TestPlugin('plugin-2', [], shouldThrowOnRegister: true);
       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));

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);
   }
 }
 

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();
     });