Skip to content

fix: activate plugin hooks before RegisterPlugin calls Register - #337

Open
abelonogov-ld wants to merge 1 commit into
mainfrom
andrey/register-plugin-hook-order
Open

fix: activate plugin hooks before RegisterPlugin calls Register#337
abelonogov-ld wants to merge 1 commit into
mainfrom
andrey/register-plugin-hook-order

Conversation

@abelonogov-ld

@abelonogov-ld abelonogov-ld commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Summary

LdClient.RegisterPlugin merged a plugin's hooks into the live pipeline only after Register returned. Construction-time registration does the opposite: GetPluginHooks feeds the Executor constructor before RegisterPlugins runs. The result was that the same plugin saw different hook behaviour depending on how it was registered, which a plugin author has no way to reason about — and which matters for plugins like observability that support both paths.

This reorders RegisterPlugin to add the hooks first. The previous ordering was deliberate and documented as a deviation, so this is a behaviour change rather than a bug fix in the narrow sense:

  • A plugin's own hooks now observe the flag evaluations and identify calls its Register makes.
  • A plugin whose Register throws now keeps its hooks, exactly as a configured plugin's do. Executor.Dispose already disposes every hook in the pipeline when the client is disposed, so the hooks are still cleaned up and DisposePluginHooks is no longer needed.
  • GetHooks throwing is unchanged: the plugin is not registered and contributes no hooks.

This brings .NET in line with the iOS, Android, and Flutter SDKs, which all activate plugin hooks before register.

Test plan

RegisterPlugin had no test coverage, so this adds it:

  • RegisterPluginRegistersPluginAndItsHooks — the client and environment metadata reach the plugin, and its hooks go live
  • RegisterPluginRunsTheRegisteringPluginsOwnHooks — a plugin that evaluates a flag inside Register sees its own hook fire, and it keeps firing afterwards
  • RegisterPluginKeepsHooksWhenRegisterThrows — hooks stay live when Register throws, and the exception does not propagate
  • RegisterPluginDoesNotRegisterPluginWhoseGetHooksThrowsRegister is not reached
  • RegisterPluginRejectsNullPlugin
  • Full client suite green: 385 passed, 0 failed

Note

Overview
LdClient.RegisterPlugin now merges plugin hooks into the live pipeline before calling Register, aligning runtime registration with plugins added via ConfigurationBuilder.Plugins (hooks first, then register). Flag evaluations and identify calls made inside Register therefore invoke that plugin’s hooks, matching iOS/Android/Flutter.

If Register throws, hooks remain active (same as a configured plugin whose Register fails); the previous DisposePluginHooks path on register failure is removed—cleanup happens when the client disposes the hook executor. GetHooks failures still abort registration with no hooks added.

Adds RegisterPlugin test coverage (hooks live, hooks during Register, Register / GetHooks failures, null rejection) plus supporting test plugins.

Reviewed by Cursor Bugbot for commit 1c0c83b. Bugbot is set up for automated code reviews on this repo. Configure here.

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 <cursoragent@cursor.com>
@abelonogov-ld
abelonogov-ld requested a review from a team as a code owner August 24, 2026 23:25
}
}

private void DisposePluginHooks(IList<Hook> pluginHooks)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants