KnownPluginList isn't thread safe

KnownPluginList isn’t thread safe. Scanning for plugins from a thread will call KnownPluginList::addType() which enters the typesArrayLock before modifying the list of types.

However, getNumTypes(), begin(), end(), and getType() that reads from the KnownPluginList don’t use the typesArrayLock. As well, the typesArrayLock is private so I can’t enter the lock myself.

So If I get a change message and then try and iterate over the KnownPluginList to see what’s up, then another plugin gets scanned as I’m still iterating, everything blows up.

6 Likes

In the short term could you expose the typesArrayLock so I can work around it myself.

Here is a minimal program that reproduces the problem: https://github.com/FigBug/juce_bugs/blob/master/ScanCrash/Source/MainComponent.cpp

I made a FakeScanner that is a subclass of KnownPluginList::CustomScanner and it returns a PluginDescription for whatever you give it. Then I just start making up possiblePluginFileOrIdentifier and scanning them as fast as I can. In the KnownPluginList changelistener, I iterate over the known descriptions, and then boom, it all blowns up within a few seconds.

Thanks for the report and test program! I’ve pushed a change to the develop branch here which aims to address these issues. Instead of using the iterators or the getType()/getTypeFor..() methods (which were inherently thread unsafe) you should now call getTypes() which return a copy of the internal description array which you are free to manipulate how you please without having to worry about threading issues or pointers becoming invalid.

2 Likes

Thanks. That looks like a pretty significant change, will take me a bit to get my main program updated. But I’ll give it a test with my sample program soon.

That is an interesting approach.

But it would be good, to get the mapping for the PopupMenu as well, so it would be consistent. It almost feel like I should rather get a copy of the whole KnownPluginList:

PopupMenu menu;
knownPluginList.addToMenu (menu, KnownPluginList::sortByCategory);
auto types = knownPluginList.getTypes();
// meanwhile scanning continues
auto menuIndex = menu.show();
auto index = knownPluginList.getIndexChosenByMenu (menuIndex);  // is this in sync?
if (isPositiveAndBelow (index, types.size())
{
    // ...
}

EDIT: it seems, if I save the getTypes() the moment I call addToMenu, and if I knew PluginTreeUtils::menuIdBase, I would have all information to safely map the result of the PopupMenu to a PluginDescription (and really, why is this called here type…? :wink: )

My suggestion would be to add the Array<PluginDescription> types as argument to addToMenu and to getIndexChosenByMenu, making them both static:

PopupMenu menu;
const auto types = knownPluginList.getTypes();
KnownPluginList::addToMenu (menu, types, KnownPluginList::sortByCategory);

// meanwhile scanning continues
auto index = KnownPluginList::getIndexChosenByMenu (menu.show(), types);
if (isPositiveAndBelow (index, types.size())
{
    // ...
}

Does that make sense? Can we use the copied result of the getTypes() in the subsequent uses of addToMenu() and getIndexChosenByMenu()?
Otherwise they will return wrong results, if the KnownPluginList is changing meanwhile!
(e.g. from a scanner in the background)

Yep, sounds sensible. I’ve pushed something here:

That’s great, thank you!