diff --git a/pkgs/sdk/client/src/LdClient.cs b/pkgs/sdk/client/src/LdClient.cs index b88178df..f0a504b0 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 b6524926..80bdc76b 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"); + } } } }