Modify EnabledModuleCachingPluginAccessor to cache getEnabledModulesByClass and fix threadsafety

Merged
#168 · Created  · Last updated

Merged pull request

Merged in issue/PLUG-1095-notracker (pull request #168)

fc4b0d1·Author: ·Closed by: ·2015-07-16

Description

  • EnabledModuleCachingPluginAccessor updated with code from Confluence to cache getEnabledModulesByClass.

  • Removed use of PluginModuleTrackers from the cache. The trackers are not fully threadsafe with respect to concurrent events in the plugin manager, when they're being created and deleted in the cache. To function correctly, they need to be populated with a consistent initial snapshot of module state and be guaranteed to receive all subsequent events; I don't think this is possible - we have no way to atomically {get-snapshot-of-state-and-register-event-listener}. So if we let trackers be their own event listener, as they are in the old version, we can't absolutely guarantee they're not returning an inconsistent state.

(This problem was worse for the ByModuleClassModuleTracker in the Confluence version - it's given a pre-built snapshot of state before it registers as an event listener, so there's an obvious window between those calls during which there might be plugin events, and it will miss them. I've deleted that class entirely. I don't like the DefaultPluginModuleTracker much either - its constructor still registers it as an event listener, which is dangerous, see comment - but I haven't changed DefaultPluginModuleTracker, just removed use of it from the EnabledModuleCachingPluginAccessor. I would like to remove it too (I've raised PLUG-1142 for this) but I suspect people are using it :) We should tell them it might occasionally lie to them...)

It turns out that removing the use of trackers makes the caching accessor simpler, and it reduces the number of event listeners in the plugin system, which is probably good.

  • Shared 'safe module extractor' code between DefaultPluginManager and EnabledModuleCachingPluginAccessor (wrapper around 'descriptor.getModule()' that disables the plugin if there's an error).

  • Made the cache listen to plugin system shutdown events (for correctness, and likely needed for tests: see here: https://extranet.atlassian.com/display/CONF/2014/05/22/Performance+-+you+never+know+where+you+can+find+gains?focusedCommentId=2234090961#comment-2234090961 )

  • Made the cache also clear itself on PluginEnabledEvent (PLUG-840)

  • Added some more tests, and fixed base class TestDefaultPluginManager to use getPluginAccessor() more, so that the subclass gets more of a workout.

  • EnabledModuleCachingPluginAccessor constructor is changed, as it now needs PluginController. (May require products to update their code that constructs it, depending on how they're doing it)

Limitations: this version goes back to the underlying plugin manager getAllEnabled... methods slightly more often (it doesn't use the info in the module enable/disable events to reload modules into the cache lists like the tracker version did; it just invalidates the cache entry and lets it be reloaded if actually accessed). Unless continuously hammered with module enable/disable events, this probably doesn't matter too much?

0 attachments

0 comments

Loading commits...