fix: activate plugin hooks before RegisterPlugin calls Register

pkgs/sdk/client/src/LdClient.cs

@@ -928,19 +928,18 @@ public Task<bool> FlushAndWaitAsync(TimeSpan timeout) =>
         /// </summary>
         /// <remarks>
         /// <para>
-        /// Retrieves hooks via <c>GetHooks</c>, calls <c>Register</c>, then merges the hooks into
-        /// the live pipeline. This ordering differs from construction-time registration for plugins
-        /// configured via <see cref="ConfigurationBuilder.Plugins"/>, where hooks are added to the
-        /// executor before <c>Register</c> is called: here, hooks are not active during
-        /// <c>Register</c>, so flag evaluations or identify calls made inside <c>Register</c> will
-        /// not invoke this plugin's hooks. After this method returns successfully, subsequent
-        /// evaluations and identify calls will invoke them.
+        /// Retrieves hooks via <c>GetHooks</c>, merges them into the live pipeline, and then calls
+        /// <c>Register</c>. This matches construction-time registration for plugins configured via
+        /// <see cref="ConfigurationBuilder.Plugins"/>, so a plugin's hooks behave the same however
+        /// it was registered: they are active during <c>Register</c>, and flag evaluations or
+        /// identify calls made inside <c>Register</c> do invoke them.
         /// </para>
         /// <para>
-        /// Exceptions thrown by the plugin's <c>Register</c> or <c>GetHooks</c> are caught and
-        /// logged; they do not propagate to the caller. If either throws, the plugin is not
-        /// registered and its hooks are not added to the live pipeline. Hooks returned by
-        /// <c>GetHooks</c> are disposed if <c>Register</c> fails.
+        /// Exceptions thrown by the plugin's <c>GetHooks</c> or <c>Register</c> are caught and
+        /// logged; they do not propagate to the caller. If <c>GetHooks</c> throws, the plugin is not
+        /// registered and contributes no hooks. If <c>Register</c> throws, the hooks are already
+        /// live and stay so, as they do for a configured plugin whose <c>Register</c> throws; they
+        /// are disposed with the client.
         /// </para>
         /// </remarks>
         /// <param name="plugin">the plugin to register; must not be null</param>
@@ -949,7 +948,7 @@ public void RegisterPlugin(Plugin plugin)
         {
             if (plugin == null) throw new ArgumentNullException(nameof(plugin));
 
-            IList<Hook> pluginHooks = null;
+            IList<Hook> pluginHooks;
             try
             {
                 pluginHooks = plugin.GetHooks(_environmentMetadata);
@@ -961,6 +960,11 @@ public void RegisterPlugin(Plugin plugin)
                 return;
             }
 
+            if (pluginHooks != null && pluginHooks.Count > 0)
+            {
+                _hookExecutor.AddHooks(pluginHooks);
+            }
+
             try
             {
                 plugin.Register(this, _environmentMetadata);
@@ -969,31 +973,6 @@ public void RegisterPlugin(Plugin plugin)
             {
                 _log.Error("Error registering plugin {0}: {1}",
                     plugin.Metadata.Name ?? "unknown", ex);
-                DisposePluginHooks(pluginHooks);
-                return;
-            }
-
-            if (pluginHooks != null && pluginHooks.Count > 0)
-            {
-                _hookExecutor.AddHooks(pluginHooks);
-            }
-        }
-
-        private void DisposePluginHooks(IList<Hook> pluginHooks)
-        {
-            if (pluginHooks == null) return;
-
-            foreach (var hook in pluginHooks)
-            {
-                try
-                {
-                    hook?.Dispose();
-                }
-                catch (Exception e)
-                {
-                    _log.Error("During disposal of hook \"{0}\" reported error: {1}",
-                        hook?.Metadata.Name, e.Message);
-                }
             }
         }
 

pkgs/sdk/client/test/LaunchDarkly.ClientSdk.Tests/LdClientPluginTests.cs

@@ -1,4 +1,6 @@
+using System;
 using System.Collections.Generic;
+using System.Collections.Immutable;
 using LaunchDarkly.Sdk.Client.Hooks;
 using LaunchDarkly.Sdk.Client.Integrations;
 using LaunchDarkly.Sdk.Client.Interfaces;
@@ -9,6 +11,8 @@
 
 namespace LaunchDarkly.Sdk.Client
 {
+    using SeriesData = ImmutableDictionary<string, object>;
+
     public class LdClientPluginTests : BaseTest
     {
         public LdClientPluginTests(ITestOutputHelper testOutput) : base(testOutput) { }
@@ -119,6 +123,95 @@ public void FailingPluginRegisterDoesNotPreventOtherPlugins()
             }
         }
 
+        [Fact]
+        public void RegisterPluginRegistersPluginAndItsHooks()
+        {
+            var hook = new RecordingHook("plugin-hook");
+            var plugin = new SpyPlugin("spy", new List<Hook> { hook });
+            var config = BasicConfig().Build();
+
+            using (var client = TestUtil.CreateClient(config, BasicUser))
+            {
+                // Nothing happens until the plugin is registered, since it was not configured.
+                Assert.False(plugin.Registered);
+
+                client.RegisterPlugin(plugin);
+
+                Assert.True(plugin.Registered);
+                Assert.Same(client, plugin.ReceivedClient);
+                Assert.Equal(BasicMobileKey, plugin.ReceivedMetadata.Credential);
+
+                client.BoolVariation("flag-key", false);
+                Assert.Equal(1, hook.BeforeEvaluationCount);
+            }
+        }
+
+        [Fact]
+        public void RegisterPluginRunsTheRegisteringPluginsOwnHooks()
+        {
+            // Evaluates a flag from inside Register, so the test can tell whether this plugin's own
+            // hooks were live at that point.
+            var hook = new RecordingHook("plugin-hook");
+            var plugin = new EvaluateOnRegisterPlugin("evaluates", hook);
+            var config = BasicConfig().Build();
+
+            using (var client = TestUtil.CreateClient(config, BasicUser))
+            {
+                client.RegisterPlugin(plugin);
+
+                // The hooks are live by the time Register runs, as they are for a configured plugin.
+                Assert.Equal(1, hook.BeforeEvaluationCount);
+
+                // And they keep running for evaluations made after registration.
+                client.BoolVariation("flag-key", false);
+                Assert.Equal(2, hook.BeforeEvaluationCount);
+            }
+        }
+
+        [Fact]
+        public void RegisterPluginKeepsHooksWhenRegisterThrows()
+        {
+            var hook = new RecordingHook("plugin-hook");
+            var plugin = new FailingPlugin("bad", new List<Hook> { hook });
+            var config = BasicConfig().Build();
+
+            using (var client = TestUtil.CreateClient(config, BasicUser))
+            {
+                // The exception is logged rather than propagated.
+                client.RegisterPlugin(plugin);
+
+                // The hooks were already live when Register threw, so they stay live, as they do for
+                // a configured plugin whose Register throws.
+                client.BoolVariation("flag-key", false);
+                Assert.Equal(1, hook.BeforeEvaluationCount);
+            }
+        }
+
+        [Fact]
+        public void RegisterPluginDoesNotRegisterPluginWhoseGetHooksThrows()
+        {
+            var plugin = new FailingGetHooksPlugin("bad-hooks");
+            var config = BasicConfig().Build();
+
+            using (var client = TestUtil.CreateClient(config, BasicUser))
+            {
+                client.RegisterPlugin(plugin);
+
+                Assert.False(plugin.Registered);
+            }
+        }
+
+        [Fact]
+        public void RegisterPluginRejectsNullPlugin()
+        {
+            var config = BasicConfig().Build();
+
+            using (var client = TestUtil.CreateClient(config, BasicUser))
+            {
+                Assert.Throws<ArgumentNullException>(() => client.RegisterPlugin(null));
+            }
+        }
+
         private class SpyPlugin : Plugin
         {
             public bool Registered { get; private set; }
@@ -152,14 +245,75 @@ private class StubHook : Hook
             public StubHook(string name) : base(name) { }
         }
 
+        /// <summary>
+        /// Counts the evaluations it sees, so a test can tell when a hook became live.
+        /// </summary>
+        private class RecordingHook : Hook
+        {
+            public int BeforeEvaluationCount { get; private set; }
+
+            public RecordingHook(string name) : base(name) { }
+
+            public override SeriesData BeforeEvaluation(EvaluationSeriesContext context, SeriesData data)
+            {
+                BeforeEvaluationCount++;
+                return data;
+            }
+        }
+
+        /// <summary>
+        /// Evaluates a flag from inside <c>Register</c>, so a test can tell whether this plugin's own
+        /// hooks were live at that point.
+        /// </summary>
+        private class EvaluateOnRegisterPlugin : Plugin
+        {
+            private readonly IList<Hook> _hooks;
+
+            public EvaluateOnRegisterPlugin(string name, Hook hook) : base(name)
+            {
+                _hooks = new List<Hook> { hook };
+            }
+
+            public override void Register(ILdClient client, EnvironmentMetadata metadata)
+            {
+                client.BoolVariation("flag-key", false);
+            }
+
+            public override IList<Hook> GetHooks(EnvironmentMetadata metadata) => _hooks;
+        }
+
         private class FailingPlugin : Plugin
         {
-            public FailingPlugin(string name) : base(name) { }
+            private readonly IList<Hook> _hooks;
+
+            public FailingPlugin(string name, IList<Hook> hooks = null) : base(name)
+            {
+                _hooks = hooks ?? new List<Hook>();
+            }
 
             public override void Register(ILdClient client, EnvironmentMetadata metadata)
             {
                 throw new System.Exception("intentional failure");
             }
+
+            public override IList<Hook> GetHooks(EnvironmentMetadata metadata) => _hooks;
+        }
+
+        private class FailingGetHooksPlugin : Plugin
+        {
+            public bool Registered { get; private set; }
+
+            public FailingGetHooksPlugin(string name) : base(name) { }
+
+            public override void Register(ILdClient client, EnvironmentMetadata metadata)
+            {
+                Registered = true;
+            }
+
+            public override IList<Hook> GetHooks(EnvironmentMetadata metadata)
+            {
+                throw new System.Exception("intentional failure");
+            }
         }
     }
 }