Fix #120: start and stop each server plugin only once

[ggiannola]

Summary

Fixes #120. A plugin whose class implements several of the plugin type interfaces was registered once per interface, so it was started and stopped multiple times.

Root cause

DefaultExtensionController.initPlugins() checks each loaded plugin against every plugin type in turn, and each of the eight branches adds the instance to the same serverPlugins collection:

if (serverPlugin instanceof ServicePlugin) { ... serverPlugins.add(servicePlugin); }
if (serverPlugin instanceof ChannelPlugin) { ... serverPlugins.add(channelPlugin); }
// ...six more

Because that collection was an ArrayList, a class implementing both ServicePlugin and ChannelPlugin ends up in it twice.

Note the issue title mentions close(); the methods actually affected are start() and stop() on ServerPlugin.

Impact beyond start/stop

The duplicate entries affect every consumer of that collection, not just shutdown:

• startPlugins() / stopPlugins() invoke start() and stop() once per implemented interface instead of once per plugin. This is the shutdown noise reported in the issue.
• AlertWorker builds its alertActionAcceptors list by iterating getServerPlugins() and filtering on instanceof AlertActionAcceptor, so a duplicated plugin is added repeatedly and its alert actions run once per implemented interface.
• DefaultUsageController reports plugin usage from the same collection, so counts are inflated for multi-interface plugins.

Fix

serverPlugins is now a Set, so a plugin registered for several interfaces is held once and the de-duplication is the natural behaviour of the collection. The eight registration calls are unchanged.

LinkedHashSet rather than HashSet: initPlugins() deliberately loads plugins in descending plugin weight order, and preserving that order means they are started and stopped in the intended sequence.

getServerPlugins() still returns List<ServerPlugin>, so the ExtensionController signature is unchanged for extensions. It now returns a copy rather than the live internal collection.

Net change is 10 insertions and 2 deletions in one file.

On testing

The earlier revision of this PR routed the registrations through a de-duplicating helper and unit tested it. Following review that has been dropped in favour of the Set, and the tests went with it: they exercised the helper directly, and the invariant is now enforced by the collection type rather than by logic of our own.

Testing this through initPlugins() instead is not really reachable from a unit test, since it requires plugin metadata loading plus database access via setPluginProperties. If you would like coverage here, I am happy to look at an integration-level test.

Verified with ./gradlew :server:test (613 tests, 0 failures) and ./gradlew :donkey:test.

Note on the failing check

The FileReceiverTest > testPoll1 failure on the previous commit is unrelated to this change, which touches only plugin registration. The assertion at FileReceiverTest.java:133 counts files picked up by a poll, and it passes locally both in the full suite and across three consecutive isolated runs. Given the recent "Allow parallel tests" change, this looks like a timing-sensitive flake rather than a real regression.

How to verify

Build a plugin whose class implements two ServerPlugin sub-interfaces, for example ServicePlugin and ChannelPlugin, and log a line in stop(). Before this change the line appears twice on server shutdown; after it, once.

[jonbartels]

I left some comments in the code with a simpler strategy for the code, though that strategy is not as easily tested.

I also left some related comments on Issue #120 to better understand the conditions.

I would be inclined to approve this PR if it implemented Set instead of an adder function that required looping over a List. TY for the PR and your first OIE commit!

[github-actions]

Test Results

655 tests  ±0   655 ✅ ±0   2m 15s ⏱️ + 1m 1s
109 suites ±0     0 💤 ±0 
109 files   ±0     0 ❌ ±0 

Results for commit 69962ae. ± Comparison against base commit 776690e.

♻️ This comment has been updated with latest results.

[ggiannola]

Thanks @jonbartels, good call on the Set, that's clearly the right shape. Pushed as 69962aef0. The helper is gone, the eight registration calls are back to plain serverPlugins.add(...), and the whole change is now +10/-2 in one file.

Two small deviations worth flagging:

• I used LinkedHashSet rather than HashSet. initPlugins() loads plugins in descending weight order and startPlugins()/stopPlugins() iterate that collection, so HashSet would randomize the start/stop sequence. LinkedHashSet keeps the Set semantics you asked for and preserves the intended order.
• getServerPlugins() still returns List<ServerPlugin> since it is declared abstract on ExtensionController and changing it would break extensions. It now returns a copy rather than the live collection.

On testing: you were right that it was awkward. The three tests exercised the helper directly, so they went with it. I think that is the better trade: with a Set the invariant is enforced by the collection type rather than by logic of ours, so there is less to regress. Going through initPlugins() is not really reachable from a unit test (it needs plugin metadata loading plus DB access via setPluginProperties), but happy to look at integration-level coverage if you would like some.

Re your question on #120 about making stop() tolerant of multiple calls — that would fix the shutdown errors, but the duplicates also reach AlertWorker (duplicate AlertActionAcceptors, so alert actions fire once per implemented interface) and DefaultUsageController (inflated plugin usage counts), which plugin-side idempotency would not help with.

One note: the FileReceiverTest > testPoll1 failure on the previous commit looks like a flake — it counts files picked up by a poll, and it passes locally both in the full suite and across three consecutive isolated runs. The new commit's build is sitting at action_required, so it will need a maintainer to approve the workflow run.