From 1c0c83b7c9a76c6a37df43a6c8488a3739eaec3c Mon Sep 17 00:00:00 2001 From: Andrey Belonogov Date: Mon, 24 Aug 2026 16:25:20 -0700 Subject: [PATCH] fix: activate plugin hooks before RegisterPlugin calls Register RegisterPlugin merged a plugin's hooks into the pipeline only after Register returned, so the same plugin saw different hook behaviour depending on whether it was configured via ConfigurationBuilder.Plugins or registered at runtime. Add the hooks first, matching construction-time registration and the iOS, Android, and Flutter SDKs. A plugin whose Register throws now keeps its hooks, as a configured plugin's do; the executor disposes them with the client, so DisposePluginHooks is no longer needed. RegisterPlugin had no test coverage, so add tests for the ordering, the metadata and hooks it passes on, and both failure paths. Co-authored-by: Cursor --- pkgs/sdk/client/src/LdClient.cs | 53 ++---- .../LdClientPluginTests.cs | 156 +++++++++++++++++- 2 files changed, 171 insertions(+), 38 deletions(-) diff --git a/pkgs/sdk/client/src/LdClient.cs b/pkgs/sdk/client/src/LdClient.cs index b88178df7..f0a504b0d 100644 --- a/pkgs/sdk/client/src/LdClient.cs +++ b/pkgs/sdk/client/src/LdClient.cs @@ -928,19 +928,18 @@ public Task FlushAndWaitAsync(TimeSpan timeout) => /// /// /// - /// Retrieves hooks via GetHooks, calls Register, then merges the hooks into - /// the live pipeline. This ordering differs from construction-time registration for plugins - /// configured via , where hooks are added to the - /// executor before Register is called: here, hooks are not active during - /// Register, so flag evaluations or identify calls made inside Register will - /// not invoke this plugin's hooks. After this method returns successfully, subsequent - /// evaluations and identify calls will invoke them. + /// Retrieves hooks via GetHooks, merges them into the live pipeline, and then calls + /// Register. This matches construction-time registration for plugins configured via + /// , so a plugin's hooks behave the same however + /// it was registered: they are active during Register, and flag evaluations or + /// identify calls made inside Register do invoke them. /// /// - /// Exceptions thrown by the plugin's Register or GetHooks 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 - /// GetHooks are disposed if Register fails. + /// Exceptions thrown by the plugin's GetHooks or Register are caught and + /// logged; they do not propagate to the caller. If GetHooks throws, the plugin is not + /// registered and contributes no hooks. If Register throws, the hooks are already + /// live and stay so, as they do for a configured plugin whose Register throws; they + /// are disposed with the client. /// /// /// the plugin to register; must not be null @@ -949,7 +948,7 @@ public void RegisterPlugin(Plugin plugin) { if (plugin == null) throw new ArgumentNullException(nameof(plugin)); - IList pluginHooks = null; + IList 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 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); - } } } diff --git a/pkgs/sdk/client/test/LaunchDarkly.ClientSdk.Tests/LdClientPluginTests.cs b/pkgs/sdk/client/test/LaunchDarkly.ClientSdk.Tests/LdClientPluginTests.cs index b65249263..80bdc76bd 100644 --- a/pkgs/sdk/client/test/LaunchDarkly.ClientSdk.Tests/LdClientPluginTests.cs +++ b/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; + 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 }); + 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 }); + 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(() => 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) { } } + /// + /// Counts the evaluations it sees, so a test can tell when a hook became live. + /// + 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; + } + } + + /// + /// Evaluates a flag from inside Register, so a test can tell whether this plugin's own + /// hooks were live at that point. + /// + private class EvaluateOnRegisterPlugin : Plugin + { + private readonly IList _hooks; + + public EvaluateOnRegisterPlugin(string name, Hook hook) : base(name) + { + _hooks = new List { hook }; + } + + public override void Register(ILdClient client, EnvironmentMetadata metadata) + { + client.BoolVariation("flag-key", false); + } + + public override IList GetHooks(EnvironmentMetadata metadata) => _hooks; + } + private class FailingPlugin : Plugin { - public FailingPlugin(string name) : base(name) { } + private readonly IList _hooks; + + public FailingPlugin(string name, IList hooks = null) : base(name) + { + _hooks = hooks ?? new List(); + } public override void Register(ILdClient client, EnvironmentMetadata metadata) { throw new System.Exception("intentional failure"); } + + public override IList 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 GetHooks(EnvironmentMetadata metadata) + { + throw new System.Exception("intentional failure"); + } } } }