Calling DirectoryContentsList::isStillLoading() is not thread safe?

I try to scan a folder using a DirectoryContentsList. It works fine in debug build, but in release build it will hang in the isStillLoading() call. Here is how I configure the DirectoryContentsList object:

DirectoryContentsList factoryPresetDirectory
factoryPresetDirectory (&filter, thread);
factoryPresetDirectory.setDirectory (getFactoryPresetFolder(), false, true);
thread.startThread (5);
 
// waiting for the directly to finish loading
while (factoryPresetDirectory.isStillLoading())
            ;

// do something with the directory 

I added some log messages in DirectoryContentsList and can confirm that the DirectoryContentsList did finish loading at some point (so the fileFindHandle is set to nullptr), but strangely in the GUI thread when calling isStillLoading, it always returns true, meaning that fileFindHandle still considered not a nullptr in the GUI thread. It only happens in Release build though. In Debug mode it works fine.

When I place a lock in DirectoryContentsList::isStillLoading() function, my app works fine in release build:

bool DirectoryContentsList::isStillLoading() const
{
    // I added a lock here
     const ScopedLock sl (fileListLock);
     
     return fileFindHandle != nullptr;
 }

It makes me wonder if there should be a lock in the function calls involving fileFindHandle? Any suggestions for fixing this issue would be very appreciated!

A while loop that does wait for something is never a good idea. It blocks the UI thread in this case and you may end up in a deadlock. You better ask this in a timer callback with some defined interval of 20ms or so.

The lock may work, but I guess your UI is blocked then until the file list is ready. That’s something we want to avoid with the separate thread you start.

I would go with the timer callback that checks if it’s loaded.

Hi @kunz, thank you for the advice! It does work with a timer. It is indeed a better design to avoid blocking the UI thread.

I’m still curious why the original code with a while loop failed to get the latest value of fileFindHandle. I will like to avoid running into the same issue again in the future. thank you!

I don’t have the time to debug this. My guess is that the DirectoryContentsList is a UI component. It loads the data in the thread you assign but maybe needs to dispatch back to the UI thread after the data is loaded but it can’t because it has to wait until your while loop is finished and the thread is free.

1 Like

It could also be because it is not atomic and only used in const methods from this thread context, so the optimizer assumes it to be constant and returns always true.

The lock you added hints to the optimizer that it might be changed from another thread.

2 Likes

That’s a good one. This would explain why it only happens in release mode.

Then this should be change in JUCE right? I had the same problem recently but quickly changed everything and it didn’t come up again.

IMHO adding a lock in this method misses the whole point of the async loading in a separate thread. It probably will block the UI thread until loading is finished. Also does the while loop that checks that flag.

That is true. I didn’t fully follow where the OP added the other lock. The lock should only protect the access of the bool, i.e. the scanner when it changes the value and the isStillLoading() when it reads.
That is how I understood the lock, but I admitt I didn’t check in the sources.
If the lock is held during the entire scan process it would indeed be bad.

TL;DR: wrapping the fileFindHandle into atomic or adding a parallel atomic_bool would be the correct solution IMHO.

3 Likes