[#1120] Apply a change to plugin-type on an enabled plugin without disabling and enabling it again

[vharseko]

Fixes #1120

Problem

When plugin-type changed on an enabled plugin, PluginConfigManager.applyConfigurationChange compared only the Java class, updated invoke-for-internal-operations and returned. The plugin stayed registered for the plugin types it was enabled with, the change was stored, and nothing was reported. The new types took effect only after the plugin was disabled and enabled again or the server was restarted. This is how #579 ended up with check-references:true that checked nothing.

Change

• PluginConfigManager.applyConfigurationChange: when the class is unchanged and the plugin types differ from existingPlugin.getPluginTypes(), the new replacePlugin() runs:

  1. it initializes a new instance of the plugin for the new plugin types (loadPlugin(..., true), the same call the "disabled → enabled" branch uses);
  2. if that fails, the change fails with the server error result code and the initialization message, and the running instance stays registered for its old plugin types;
  3. otherwise, under pluginLock, it takes the running instance out of the plugin arrays and registers the new one in its place. Only after that, outside the lock, does it call finalizePlugin() of the old instance. An exception from that call is logged, as in finalizePlugins().

  A change of the Java class keeps the current behaviour (adminActionRequired). invoke-for-internal-operations is now applied to the running instance before the plugin types are compared, so it also takes effect when the new types are refused.

• loadPlugin(): when initializePlugin() throws, the failed instance is finalized (quietly). A plugin that registered a change listener or started a thread before it failed no longer leaves it behind. This covers startup, add, enable and the replacement.

• The eight invokePreOperation*Plugins methods read the plugin array into a local once. Before, they read the field again for the loop bound, the element and the array passed to handlePreOperation* / registerSkippedPreOperationPlugins, so a concurrent registration change could skip a plugin or throw ArrayIndexOutOfBoundsException. This predates the PR, but a plugin-type change is one more way to trigger it.

• SambaPasswordPlugin, PasswordPolicyImportPlugin and MsadPlugin now remove their change listener in finalizePlugin(). Before, a finalized instance kept listening to the entry after every disable/enable, and for Samba and MSAD also after every plugin type change. For MSAD, a refused plugin type also left the refused instance listening.

• ReferentialIntegrityPlugin registers its change listener only after its configuration check, and finalizePlugin() returns early when the check refused the configuration. Before, the listener was registered first. finalizePlugin() then failed with a NullPointerException before it removed it. So an instance refused when the server started, where there is no acceptance phase, kept listening to its entry, and a later change of update-interval could start its background thread.

• The DirectoryServerPlugin.finalizePlugin() Javadoc now says that the method is also called on an instance whose initializePlugin() threw.

• PluginConfiguration.xml: plugin-type no longer declares component-restart, so dsconfig help and the configuration reference no longer say the change needs the component to be restarted.

• chap-writing-plugins.adoc: a paragraph for plugin authors on how a change to plugin-type re-creates the plugin, and on what finalizePlugin() must release when initializePlugin() fails.

Why a new instance, not the running one moved to other types

A plugin receives its plugin types only in initializeInternal() / initializePlugin(). DirectoryServerPlugin.pluginTypes has no setter, and it is public plugin API. The default hooks throw UnsupportedOperationException, and the default isConfigurationAcceptable() returns true. So a third-party plugin that checks its types only in initializePlugin() passes the acceptance phase. Moved in place, it would then fail every operation of a type it does not support. A new instance goes through initializePlugin(), and a refusal is caught before anything is registered.

The configuration change listeners handle the swap. ConfigurationHandler.replaceEntry iterates a snapshot of the listeners and skips the ones that are no longer registered. The in-tree plugins remove their listener in finalizePlugin(), so the old instance is not called for the change after it is finalized. The new instance's listener is not in the snapshot, but that instance was already initialized with the new configuration. The manager's own listener comes first in the list, because it is registered before the plugin's.

Trade-offs

• The new instance is initialized before the old one is taken out, so a refusal leaves the plugin working. The plugin is never registered twice: the old instance is out of the arrays before the new one goes in. The remaining gap is the rewrite of the plugin arrays, for all of the plugin's types. It does not include the old instance's finalizePlugin(), which for the referential integrity plugin with update-interval > 0 waits for the background thread.
• State the plugin keeps in memory is not carried over, just as with disable/enable. For example, the uniqueness reservations of UniqueAttributePlugin or the background thread of the referential integrity plugin. With update-interval > 0, the old and new referential integrity instances briefly hold the same log file. Each synchronizes on its own File object, so a delete logged by the new instance while the old thread is still in processLog can be lost. With the order of the first round, where the old instance was finalized first, such a delete was not logged at all. The new background thread first processes the file after one update interval.
• As in the existing "disabled → enabled" branch, the configuration is written before the listeners run. On a refusal the stored plugin-type differs from what is running, and every later change of the entry retries the replacement and fails with the same reason until plugin-type is set to types the plugin accepts. The other properties of such a change are still applied to the running instance.

Interaction with #1123

#1123 was merged first. It told the administrator to disable and re-enable the referential integrity plugin after adding plugin types. The advice was in chap-groups.adoc ("Plugin types take effect when the plugin is enabled, so add them before enabling the plugin, or disable and re-enable it afterwards.") and in ERR_PLUGIN_REFERENT_CHECK_REFERENCES_WITHOUT_PLUGIN_TYPE / WARN_PLUGIN_REFERENT_CHECK_REFERENCES_WITHOUT_PLUGIN_TYPE. With this change, added plugin types take effect at once. So the sentence now says so, and both messages end with "Add '…' to 'plugin-type', or set 'check-references' to false".

Tests

• PluginConfigManagerTestCase. The test plugin PluginTypeTrackingPlugin counts pre- and post-operation modify invocations per entry, registers a change listener in initializePlugin() before it checks its types, and removes it in finalizePlugin(). It records finalizePlugin(), which plugin was registered for its entry at that moment, and whether its listener received a change after it was finalized. It refuses PRE_OPERATION_DELETE only in initializePlugin(), and a static switch makes finalizePlugin() throw.
  • testPluginTypeChangeAppliesToEnabledPlugin: adding preOperationModify to the enabled plugin gives it a new instance with the new types, which is invoked. The old instance is finalized after the new one is registered, the number of change listeners on the entry does not change, and no finalized instance receives a change. A change that leaves the plugin types alone keeps the running instance. Removing the type again works the same way.
  • testPluginTypeChangeRefusedByPluginKeepsRunningPlugin: a change to a type the plugin refuses fails. The running instance stays registered, not finalized, with its types, and is still invoked. The refused instance leaves no listener behind. A later change of invoke-for-internal-operations still fails, but is applied to the running instance.
  • testPluginTypeChangeWhenOldInstanceFailsToFinalize: when the old instance's finalizePlugin() throws, the change succeeds and the new instance stays registered.
  • testPluginTypeChangeWithJavaClassChangeKeepsRunningPlugin: a modify that changes java-class (to OtherPluginTypeTrackingPlugin) together with plugin-type leaves the running instance and its types unchanged.
  • Every modify of the test entry must invoke the plugin once in post-operation.
• ReferentialIntegrityPluginTestCase.testEnforceIntegrityAfterPluginTypesAddedToEnabledPlugin: the scenario from the issue. While the plugin stays enabled, preOperationAdd / preOperationModify are added and check-references is set. Adding a group with a missing member must then be refused.
• ReferentialIntegrityPluginTestCase.testConfigurationRefusedByInitializePluginLeavesNoChangeListenerBehind: for a configuration refused by the plugin's check and for one refused when the log file is set up, initializePlugin() followed by finalizePlugin(), as loadPlugin() does, leaves the number of change listeners on the entry unchanged and does not throw.
• MsadPlugin has no test: its module has no test support, and the same change in SambaPasswordPlugin is pinned.
• SambaPasswordPluginTestCase.testPluginTypeChangeLeavesNoChangeListenerBehind and PasswordPolicyImportPluginTestCase.testDisableAndEnableLeavesNoChangeListenerBehind: the number of change listeners on the entry does not change across a plugin type change and a disable/enable.

Local runs, in the reactor on top of #1123:

• GREEN: PluginConfigManagerTestCase 38/38, ReferentialIntegrityPluginTestCase 61/61 (with the [#1118] Register the shipped Referential Integrity plugin for the pre-operation types check-references needs, and refuse check-references without them #1123 cases), SambaPasswordPluginTestCase 25/25, PasswordPolicyImportPluginTestCase 59/59. opendj-server-msad-plugin builds.
• Mutants, each caught:
  • the first-round head of this PR (3 cases fail), and its referential integrity plugin against the second-round test (NPE);
  • ReferentialIntegrityPlugin registering its listener before the configuration check, or finalizePlugin() without the null guard;
  • re-creating the plugin on a change that leaves the plugin types alone;
  • finalizing the old instance before the new one is registered;
  • letting an exception of the old instance's finalizePlugin() escape;
  • not finalizing the old instance;
  • not finalizing an instance whose initializePlugin() failed;
  • applying invoke-for-internal-operations only when the plugin types are unchanged;
  • replacing the plugin when the Java class changes too;
  • registering the new instance only for the added plugin types;
  • SambaPasswordPlugin / PasswordPolicyImportPlugin without finalizePlugin().
    The snapshot of the pre-operation arrays has no test: the race needs a registration change between two reads inside one invocation.
• Regression on top of [#1118] Register the shipped Referential Integrity plugin for the pre-operation types check-references needs, and refuse check-references without them #1123: DirectoryServerPluginTestCase 50, AttributeCleanupPluginTestCase 11, EntryUUIDPluginTestCase 61, LDAPADListPluginTestCase 58, LastModPluginTestCase 61, SevenBitCleanPluginTestCase 12, UniqueAttributePluginTestCase 16 pass. In the first round, before the rebase, AddOperationTestCase 138, ModifyOperationTestCase 936, DeleteOperationTestCase 195 and SearchOperationTestCase 75 passed too.

[maximthomas]

praise: The fix sits where the bug is and fails safe.

• replacePlugin initializes the new instance before it touches the running one, so a refusal in initializePlugin() leaves the plugin working (PluginConfigManager.java:4553-4562).
• component-restart is dropped from plugin-type in PluginConfiguration.xml in the same change, so dsconfig help stops contradicting the code.
• PluginTypeTrackingPlugin refuses its types only in initializePlugin(), which is exactly the case the acceptance phase cannot catch.

---

suggestion (non-blocking): Register the new instance before finalizing the old one, so the plugin is never missing for the length of finalizePlugin().

opendj-server-legacy/src/main/java/org/opends/server/core/PluginConfigManager.java:4567-4568

deregisterPlugin(dn) removes the old instance from every type array, the unchanged types included, and then calls finalizePlugin(). registerPlugin runs only after that. The invoke* methods read the arrays without pluginLock, so for the whole of finalizePlugin() the plugin runs for no type. For ReferentialIntegrityPlugin with update-interval > 0, finalizePlugin → processServerShutdown joins the background thread, and the join waits for a running processLog to finish. A delete arriving in that time is never logged, so its references are never cleaned, and check-references is skipped. The description calls this "the gap between removing the old instance and adding the new one, per plugin type". In fact it covers all types and lasts as long as the finalize. Finalizing after registering keeps the "never invoked twice" property, because the old instance is already out of the arrays, and it shrinks the gap to the array writes.

    pluginLock.lock();
    try
    {
      DirectoryServerPlugin<? extends PluginCfg> oldPlugin = registeredPlugins.remove(configuration.dn());
      if (oldPlugin != null)
      {
        deregisterPlugin0(oldPlugin);
      }
      registerPlugin(plugin, configuration.dn(), pluginTypes);
      if (oldPlugin != null)
      {
        oldPlugin.finalizePlugin();
      }
    }
    finally
    {
      pluginLock.unlock();
    }

RI's writeLog/processLog lock each instance's own File (ReferentialIntegrityPlugin.java:789, :813, :840). So after this change a delete logged by the new instance while the old thread is still in processLog can still be lost. That is no worse than today, where the delete is not logged at all.

---

issue (non-blocking): If the old instance's finalizePlugin() throws, the plugin ends up registered in neither form.

opendj-server-legacy/src/main/java/org/opends/server/core/PluginConfigManager.java:4567

deregisterPlugin calls finalizePlugin() without a catch, and the try/finally in replacePlugin only unlocks. A RuntimeException there skips registerPlugin. The entry is then gone from registeredPlugins and from every array. The new instance, with its listeners and threads already started, is never registered, and the modify fails with the unchecked exception. No in-tree finalizePlugin throws, so only a faulty third-party plugin gets here. The reorder above fixes it: with registerPlugin before finalizePlugin(), a throwing finalize no longer removes the plugin.

---

question (non-blocking): After a refused change, should an unrelated later modify of the entry still fail?

opendj-server-legacy/src/main/java/org/opends/server/core/PluginConfigManager.java:4499-4503

replaceEntry stores the refused plugin-type before the listeners run. After that, every later change compares the stored types with the running instance's types, retries replacePlugin, and returns before setInvokeForInternalOperations. So after the refused modify in testPluginTypeChangeRefusedByPluginKeepsRunningPlugin, replace: ds-cfg-invoke-for-internal-operations: false fails with the plugin-type error. It is stored anyway, and the running plugin keeps running for internal operations. At BASE the same modify succeeded. The description says the stored and running types differ "until the next change", but for an unrelated next change they still differ, and that change fails too. Minor either way. If it is intended, a sentence in the description or the dev guide covers it. If not, the unrelated properties could still be applied to the running instance on the refusal road.

---

issue (non-blocking): On the refusal road, whatever the rejected instance registered before initializePlugin() threw stays registered next to the running instance.

opendj-server-legacy/src/main/java/org/opends/server/core/PluginConfigManager.java:4557-4562, opendj-server-legacy/src/main/java/org/opends/server/plugins/ReferentialIntegrityPlugin.java:169, :180

ReferentialIntegrityPlugin.initializePlugin adds its change listener first. It throws only later, in setUpLogFile, when the log file cannot be created. isConfigurationAcceptable never reads log-file. So a modify that changes plugin-type and sets log-file under a missing directory passes acceptance and then fails in apply. The rejected instance then stays registered as a listener on the same DN, and every later change of the entry retries and adds one more. loadPlugin has never finalized a failed instance: the add and enable roads do the same at BASE, but there no live instance sits beside it. A guarded finalizePlugin() on the failure path in loadPlugin would fix all three roads. A follow-up is fine.

---

issue (non-blocking): SambaPasswordPlugin and PasswordPolicyImportPlugin never remove their change listener, so each replace leaves a finalized instance listening on the entry.

opendj-server-legacy/src/main/java/org/opends/server/plugins/SambaPasswordPlugin.java:862, opendj-server-legacy/src/main/java/org/opends/server/plugins/PasswordPolicyImportPlugin.java:109

Both plugins add their listener in initializePlugin and do not override finalizePlugin, whose default does nothing. The description says "the old instance removes its listener in finalizePlugin(), so it is not called for the change after it is finalized". That is not true for these two. The old listener is still registered, so it is in replaceEntry's snapshot, and the finalized instance receives this change and every later one. Each plugin-type change adds one more. This is harmless today and the same leak already happens on disable/enable. You could fix it here with a finalizePlugin that removes the listener, or narrow the sentence in the description.

---

issue (non-blocking): The indexed invokePreOperation*Plugins loops read the array field again on every iteration, so a swap in the middle of an operation can skip an unrelated plugin or throw ArrayIndexOutOfBoundsException.

opendj-server-legacy/src/main/java/org/opends/server/core/PluginConfigManager.java:2325-2358 (and the other seven pre-operation loops)

The field is plain and is read in the loop condition, in the element access, and again when it is passed to handlePreOperationException / registerSkippedPreOperationPlugins, all without a lock. If X is removed from a slot before i, the next read skips the plugin that was at i. If the length comes from the old array and the element from the shorter new one, the access throws. This predates the PR: enable, disable, add and delete already reach it. The PR adds a plugin-type change as one more trigger. A follow-up is fine.

    DirectoryServerPlugin[] plugins = preOperationAddPlugins;
    for (int i = 0; i < plugins.length; i++)
    {
      DirectoryServerPlugin p = plugins[i];
      // ... and pass plugins, not the field, to handlePreOperationException,
      // handlePreOperationResult and registerSkippedPreOperationPlugins

---

suggestion (non-blocking): The countChangeListeners(pluginDN) asserts cannot fail on a plugin-instance listener fault, because the test plugin registers no listener.

opendj-server-legacy/src/test/java/org/opends/server/core/PluginConfigManagerTestCase.java:673, :680

PluginTypeTrackingPlugin adds no change listener, so the only listener on the entry is the manager's, and replacePlugin never touches it. If an old instance kept its listener, or replaceEntry called a finalized instance, both asserts would still compare 1 with 1. The description cites these asserts for "the number of change listeners on the entry does not change". The finalizePlugin() mutant is killed, but by isFinalized().

  private volatile PluginCfg configuration;
  private static final AtomicInteger CHANGES_SEEN_WHEN_FINALIZED = new AtomicInteger();
  private final ConfigurationChangeListener<PluginCfg> listener = new ConfigurationChangeListener<PluginCfg>()
  {
    @Override
    public boolean isConfigurationChangeAcceptable(PluginCfg cfg, List<LocalizableMessage> reasons)
    {
      return true;
    }

    @Override
    public ConfigChangeResult applyConfigurationChange(PluginCfg cfg)
    {
      if (finalized)
      {
        CHANGES_SEEN_WHEN_FINALIZED.incrementAndGet();
      }
      return new ConfigChangeResult();
    }
  };

  // initializePlugin(): this.configuration = configuration; configuration.addChangeListener(listener);
  // finalizePlugin():   configuration.removeChangeListener(listener); finalized = true;

Pin: with the listener in place, the unchanged count covers the plugin's own listener, and CHANGES_SEEN_WHEN_FINALIZED == 0 after each replace pins the snapshot skip the description relies on.

---

suggestion (non-blocking): No test covers the rule that a java-class change in the same modify leaves the plugin types unapplied.

opendj-server-legacy/src/main/java/org/opends/server/core/PluginConfigManager.java:4497

If the else is dropped, a modify that changes java-class and plugin-type together calls replacePlugin. That loads the new class at once, which contradicts "a change of the Java class keeps the current behaviour". The mutant survives: the new cases change plugin-type alone, and no test modifies ds-cfg-java-class on an existing entry.

    ModifyRequest request = newModifyRequest(pluginDN)
        .addModification(REPLACE, "ds-cfg-java-class", PluginTypeTrackingPlugin2.class.getName())
        .addModification(REPLACE, "ds-cfg-plugin-type", "postOperationModify", "preOperationModify");
    getRootConnection().processModify(request);
    assertSame(getTrackingPlugin(pluginDN), original);
    assertFalse(original.isFinalized());
    assertEquals(original.getPluginTypes(), EnumSet.of(POST_OPERATION_MODIFY));

Pin: PluginTypeTrackingPlugin2 is a trivial top-level subclass of PluginTypeTrackingPlugin. assertSame fails against the mutant.

---

suggestion (non-blocking): The replacement's post-operation registration is never observed.

opendj-server-legacy/src/test/java/org/opends/server/plugins/PluginTypeTrackingPlugin.java:78

doPostOperation records nothing, and getPluginTypes() returns the set handed to initializeInternal, not what the manager registered. Take a replacePlugin that registers only the added types. On {POST}→{POST,PRE} it registers only PRE. On {POST,PRE}→{POST} it registers nothing, so the plugin runs for no type. Both cases, the refused case and the RI case all stay green against it.

  private static final ConcurrentHashMap<DN, AtomicInteger> POST_OPERATION_MODIFY_COUNTS = new ConcurrentHashMap<>();

  @Override
  public PluginResult.PostOperation doPostOperation(PostOperationModifyOperation modifyOperation)
  {
    POST_OPERATION_MODIFY_COUNTS.computeIfAbsent(modifyOperation.getEntryDN(), dn -> new AtomicInteger())
        .incrementAndGet();
    return PluginResult.PostOperation.continueOperationProcessing();
  }

Pin: add a takePostOperationModifyCount and assert 1 in modifyTestEntryAndCountPreOperation. Against the mutant, the post-operation count after the first change is 0.

[vharseko]

All nine points are addressed in aafa02d. The branch is also rebased onto the current master (#1122, #1124). There were no conflicts, and the first commit is unchanged.

1. Register before finalize. Done, as in your snippet, with one difference: the old instance is finalized after pluginLock is released, so the RI join does not hold the lock. The old instance is already out of registeredPlugins and the arrays, so nothing else reaches it. Pinned: the test plugin records getRegisteredPlugin(dn) inside finalizePlugin(), and the old instance must see its replacement there. The mutant that finalizes before registering fails on that assert.

2. Throwing finalizePlugin(). The reorder alone would keep the plugin registered, but the unchecked exception would still fail the modify after the swap had happened. So the call goes through finalizePluginQuietly() (traceException, like finalizePlugins()). New case testPluginTypeChangeWhenOldInstanceFailsToFinalize. The previous head, the "finalize first" mutant and the "let it escape" mutant each fail it.

3. Unrelated change after a refusal. Not intended. setInvokeForInternalOperations now runs before the class and type checks, so it reaches the running instance on every path. The change itself still fails as long as the stored plugin-type cannot be applied. I kept that on purpose: the stored configuration is not what is running, and the retry picks up a cause that has gone away. The description and the dev guide paragraph now say so. The refused case asserts invokeForInternalOperations() is false after such a modify.

4. Leftovers of the rejected instance. Fixed here, not as a follow-up, because of (3): every later change retries, so a leaked instance would pile up once per change. The RI zombie is also not quite harmless. It throws in setUpLogFile before interval is set, so interval stays 0, and a later change with update-interval > 0 makes its applyConfigurationChange start a background thread and register a shutdown listener. loadPlugin now finalizes the instance quietly when initializePlugin() throws, which covers startup, add, enable and the replacement. The test plugin registers its listener before it refuses, and the refused case asserts the listener count, after the refusal and after the retry.

5. Samba / PasswordPolicyImport listeners. Both now remove their listener in finalizePlugin(). A detail: PasswordPolicyImportPlugin cannot reach the replacement road. It accepts only ldifImport, and its isConfigurationChangeAcceptable rejects any other type in the acceptance phase, so for it the leak happened only on disable/enable. Samba can reach it (preOperationModify / postOperationExtended). Tests: SambaPasswordPluginTestCase.testPluginTypeChangeLeavesNoChangeListenerBehind (type change) and PasswordPolicyImportPluginTestCase.testDisableAndEnableLeavesNoChangeListenerBehind. Each fails without the respective finalizePlugin().

6. Indexed pre-operation loops. Fixed here as well: the eight methods read the array into a local once and pass that local on. There is no test, because the race needs a registration change between two reads inside one invocation. Side note, left as it is: invokePreOperationComparePlugins never calls registerSkippedPreOperationPlugins.

7. Listener in the test plugin. Done along the lines of your snippet: the listener counts changes seen after finalizePlugin(), and the applies case asserts 0 after the second replacement.

8. java-class + plugin-type. New case testPluginTypeChangeWithJavaClassChangeKeepsRunningPlugin with OtherPluginTypeTrackingPlugin. The mutant without the else fails it.

9. Post-operation registration. doPostOperation counts, and modifyTestEntryAndCountPreOperation asserts one post-operation invocation per modify. The "register only the added types" mutant fails three cases.

Local runs: PluginConfigManagerTestCase 38/38, ReferentialIntegrityPluginTestCase 50/50, SambaPasswordPluginTestCase 25/25, PasswordPolicyImportPluginTestCase 59/59. Ten mutants all fail (listed in the description). The plugin and core operation regression classes pass.

[maximthomas]

praise: The second commit closes every first-round point where the bug is, and pins most of them.

• replacePlugin registers the new instance before it finalizes the old one, outside pluginLock and through finalizePluginQuietly; the test plugin records getRegisteredPlugin(dn) inside finalizePlugin(), so getRegisteredWhenFinalized() pins the order.
• The eight invokePreOperation*Plugins methods read the array once and pass that same local to handlePreOperation* and registerSkippedPreOperationPlugins.
• setInvokeForInternalOperations now runs before the class and type checks, pinned by assertFalse(running.invokeForInternalOperations()) in the refused case.

---

issue (non-blocking): The finalize-on-failed-init in loadPlugin does not remove the change listener of ReferentialIntegrityPlugin when RI's own configuration check fails at startup.

opendj-server-legacy/src/main/java/org/opends/server/plugins/ReferentialIntegrityPlugin.java:169, :174, :889, opendj-server-legacy/src/main/java/org/opends/server/core/PluginConfigManager.java:408

initializePlugin adds its listener at :169 and throws at :174 when isConfigurationAcceptable fails. currentConfiguration is assigned only at :250, inside applyConfigurationChange, which runs after that throw. So finalizePlugin() throws an NPE at :889 before it removes anything, finalizePluginQuietly only traces the NPE, and the listener stays. Only startup reaches this, because initializeUserPlugins has no acceptance phase; add, enable and replace run the same check first. Example: an RI attribute-type whose equality index was removed before a restart. The plugin is logged and skipped, but the skipped instance keeps listening, and with update-interval > 0 a later change of the entry makes it start a background thread. The leak predates the PR. The commit message and the description say the finalize "covers startup, add, enable and the replacement", and the new dev-guide paragraph asks plugin authors for exactly what RI does not do. The setUpLogFile failure is covered: currentConfiguration and interval are set by then.

    LinkedList<LocalizableMessage> unacceptableReasons = new LinkedList<>();

    if (!isConfigurationAcceptable(pluginCfg, unacceptableReasons))
    {
      throw new ConfigException(unacceptableReasons.getFirst());
    }

    // Only after the check, so that a refused configuration leaves no listener behind.
    pluginCfg.addReferentialIntegrityChangeListener(this);
    applyConfigurationChange(pluginCfg);

Or, in addition: guard finalizePlugin() with if (currentConfiguration != null). That way the refused road does not throw an NPE that is only traced.

---

issue (non-blocking): MsadPlugin never removes its change listener, so every plugin-type change and every refused retry leaves a stale listener.

opendj-server-msad-plugin/src/main/java/opendj/MsadPlugin.java:68, :86, :189-192

The plugin adds config.addMsadChangeListener(this) before its type switch throws for anything outside bind, add and modify. It overrides neither finalizePlugin() nor isConfigurationAcceptable(). When the types change from {preOperationBind} to {preOperationBind, preOperationAdd}, replacePlugin finalizes the old instance with the no-op default, and its listener stays. A refused type, such as postOperationAdd, passes the acceptance phase and leaks the refused instance's listener, and every later change of the entry repeats that. From then on each stale instance logs "changed MSAD plugin configuration" on every change and stays in memory until restart. The PR fixes this for SambaPasswordPlugin and PasswordPolicyImportPlugin. This one is in another module, and my first round missed it. The removal matches by config DN, so the cfg object that applyConfigurationChange replaced still works.

    @Override
    public void finalizePlugin() {
        config.removeMsadChangeListener(this);
    }

---

suggestion (non-blocking): No case pins that a change which leaves the plugin types alone keeps the running instance.

opendj-server-legacy/src/main/java/org/opends/server/core/PluginConfigManager.java:4558, opendj-server-legacy/src/test/java/org/opends/server/core/PluginConfigManagerTestCase.java:678

Suppose the guard !pluginTypes.equals(existingPlugin.getPluginTypes()) is dropped. Then every change of an enabled plugin's entry re-creates the plugin and drops its in-memory state. Every modify in the new cases already runs replacePlugin with or without the guard: it changes the types, it runs while refused types are stored (the invoke-for-internal-operations modify at :724), or it changes the class. No other case I traced holds an instance across a same-type config modify. This was traced, not run. The RI, SevenBitClean, UniqueAttribute and AttributeCleanup suites were not traced.

      // A change that leaves the plugin types alone keeps the running instance.
      assertEquals(getRootConnection().processModify(newModifyRequest(pluginDN)
          .addModification(REPLACE, "ds-cfg-invoke-for-internal-operations", "false")).getResultCode(),
          ResultCode.SUCCESS);
      assertSame(getTrackingPlugin(pluginDN), replacement);
      assertFalse(replacement.isFinalized());
      assertEquals(getRootConnection().processModify(newModifyRequest(pluginDN)
          .addModification(REPLACE, "ds-cfg-invoke-for-internal-operations", "true")).getResultCode(),
          ResultCode.SUCCESS);

Pin: in testPluginTypeChangeAppliesToEnabledPlugin after :678. With the guard dropped, the first modify re-creates the plugin and assertSame fails.

---

nitpick (non-blocking): The DirectoryServerPlugin.finalizePlugin() Javadoc still says it runs only after the plugin is deregistered.

opendj-server-legacy/src/main/java/org/opends/server/api/plugin/DirectoryServerPlugin.java:167-170

loadPlugin now also calls it on an instance whose initializePlugin() threw and that was never registered. Only chap-writing-plugins.adoc states the new duty, and the RI issue above is an in-tree plugin that does not meet it.

  /**
   * Performs any necessary finalization for this plugin.  This will
   * be called just after the plugin has been deregistered with the
   * server but before it has been unloaded.  It is also called when
   * {@link #initializePlugin} throws, on an instance that was never
   * registered: it must then release whatever {@code initializePlugin}
   * acquired before it failed, and must not assume that it completed.
   */

[vharseko]

All four points are addressed in d4b0749. The branch is also rebased onto the current master, which now has #1123. There were no conflicts, and only the base of the first two commits changed.

1. RI listener on a configuration refused at startup. Done as in your snippet, plus the guard. initializePlugin registers the listener only after isConfigurationAcceptableIgnoringCheckReferencesPluginTypes (the startup check after #1123), and finalizePlugin() returns early when currentConfiguration is null. I checked the other in-tree plugins. RI was the only one that registered its listener before a check that could fail with the configuration field still unset. SambaPasswordPlugin and AttributeCleanupPlugin register theirs after the check, so there finalizePlugin() only hit a traced NPE. Samba's finalizePlugin() is new in this PR, so I gave it the same guard. New case testConfigurationRefusedByInitializePluginLeavesNoChangeListenerBehind. It runs two configurations: one refused by the check and one refused in setUpLogFile. It counts the change listeners on the entry around initializePlugin + finalizePlugin(), as loadPlugin does. The previous head fails it with the NPE. Two mutants fail it too: "listener before the check" (one listener too many) and "no guard" (the NPE). The case uses its own DN. With the real one, an instance left behind by a mutant refused the check-references change in the next #1123 case.

2. MsadPlugin. finalizePlugin() removes the listener. config is set before the listener is added, so this covers both the replaced instance and an instance that refuses its types. There is no test: the module has none, and building test support for it would be larger than the fix. The same change in SambaPasswordPlugin is pinned by its test. The module now builds in the reactor with the change. The file had no license header, so it now has the CDDL one.

3. Same plugin types keep the instance. Your snippet is in testPluginTypeChangeAppliesToEnabledPlugin, with an invokeForInternalOperations() check and a small helper, which the refused case uses too. The mutant that drops !pluginTypes.equals(...) fails it at assertSame.

4. Javadoc. Your text is in DirectoryServerPlugin.finalizePlugin().

Also, after the rebase onto #1123. As the description said, the PR merged second has to remove the "disable and re-enable" sentence. It is replaced in chap-groups.adoc. The same advice was also in both #1123 messages, ERR_PLUGIN_REFERENT_CHECK_REFERENCES_WITHOUT_PLUGIN_TYPE and WARN_…, so it is dropped there too. Both now say "Add '…' to 'plugin-type', or set 'check-references' to false".

Local runs, reactor with #1123: PluginConfigManagerTestCase 38/38, ReferentialIntegrityPluginTestCase 61/61 (with the #1123 cases), SambaPasswordPluginTestCase 25/25, PasswordPolicyImportPluginTestCase 59/59. opendj-server-msad-plugin builds. Three mutants (listed above) each fail.

[maximthomas]

praise: The new commit closes every round-2 point the way it was offered, and pins the main one.

• ReferentialIntegrityPlugin registers its change listener only after the configuration check (ReferentialIntegrityPlugin.java:177-182), finalizePlugin() returns early on a refused configuration (:934-938), and testConfigurationRefusedByInitializePluginLeavesNoChangeListenerBehind counts the entry's listeners on both refusal roads (the check and setUpLogFile).
• MsadPlugin.finalizePlugin() (MsadPlugin.java:109-113) removes the last in-tree plugin listener that leaked on finalize, and the DirectoryServerPlugin.finalizePlugin() Javadoc (:171-175) now states the failed-init call.
• The #1123 texts that told the operator to disable and re-enable the plugin after adding plugin types are updated (plugin.properties:357-365, chap-groups.adoc:542), and PluginConfigManagerTestCase:680-686 pins that a modify leaving the plugin types alone keeps the running instance.