juce-framework / JUCE

JUCE is an open-source cross-platform C++ application framework for desktop and mobile applications, including VST, VST3, AU, AUv3, LV2 and AAX audio plug-ins.
https://juce.com
Other
6.62k stars 1.75k forks source link

[Bug]: using ThreadSafeListener still triggers assert when calling from 2 threads #1450

Open benkuper opened 1 week ago

benkuper commented 1 week ago

Detailed steps on how to reproduce the bug

using ThreadSafeListener should prevent from triggering an assert when calling from multiple threads (as per indication in the assert comments), but it still trigger it.

_EDIT : Reading the ListenerList function where it happens, the ScopedTryLock on line 233 of juceListenerList.h is intriguing to me. Shouldn't there be a function that blocks and ensure the lock is gained (so ScopedLock) when we're using CriticalSection ? Right now, I don't really see what's the actual function of ThreadSafe except telling me that it may not be safe.

Minimal test case :

This is a visual representation of the above description :

Flowchart

I believe this should not trigger the assert, and in scenarios where calls are done repeatedly, this makes impossible to debug as the assert will stop the program every time.

here is the full code :

#include <JuceHeader.h>
#include "MainComponent.h"

class ScriptTestApplication;

class ThreadTest : public juce::Thread
{

public:
    ScriptTestApplication* test;

    ThreadTest(ScriptTestApplication * test) : juce::Thread("ThreadTest"), test(test)
    {
        startThread();
    }

    ~ThreadTest()
    {
        signalThreadShouldExit();
        waitForThreadToExit(-1);
    }

    void run() override;
};

class TestListener
{
public:
    virtual ~TestListener() {}
    virtual void notified() = 0;
};

class ScriptTestApplication : public juce::JUCEApplication, public TestListener
{
public:
    ScriptTestApplication() :
        threadTest1(this),
        threadTest2(this)
    {}

    const juce::String getApplicationName() override { return ProjectInfo::projectName; }
    const juce::String getApplicationVersion() override { return ProjectInfo::versionString; }
    bool moreThanOneInstanceAllowed() override { return true; }

    ThreadTest threadTest1;
    ThreadTest threadTest2;
    juce::ThreadSafeListenerList<TestListener> listeners;

    void initialise(const juce::String& commandLine) override
    {
        listeners.add(this);
    }

    void notify()
    {
        DBG("Notify : " << (int)juce::Thread::getCurrentThreadId());
        listeners.call(&TestListener::notified);
    }

    void notified()
    {
        DBG(" > Notified : " << (int)juce::Thread::getCurrentThreadId());
    }

    void shutdown() override
    {
    }

    //==============================================================================
    void systemRequestedQuit() override
    {
        quit();
    }

    void anotherInstanceStarted(const juce::String& commandLine) override
    {

    }

};
START_JUCE_APPLICATION(ScriptTestApplication)

void ThreadTest::run()
{
    while (!threadShouldExit())
    {
        test->notify();
        wait(2);
    }
}

Operating systems

Windows

What versions of the operating systems?

11

Architectures

64-bit

Stacktrace

No response

Plug-in formats (if applicable)

No response

Plug-in host applications (DAWs) (if applicable)

No response

Testing on the develop branch

The bug is present on the develop branch

Code of Conduct

benkuper commented 1 week ago

I made a PR that proposes a ThreadSafeBlockListenerList, and that uses ScopedLock instead of ScopedTryLock After testing in both my minimal setup and my full-blown software with a lot of cross calls between listeners and threads, this seems very stable.

If you're interested in the context from which I find and publish all the issues, this is the software I develop and test Juce 8 against : https://benjamin.kuperberg.fr/chataigne