I agree, probably not the most elegant solution.
I wonder if there’s something you could do with juce::WaitableEvents, but maybe that’s just the same issue in a different box.
Not exactly atomic but safe because you hold a lock and therefore the queue is either empty and will be started at the next add job or it is not empty and continues to be processed.
If you don’t hold the lock, how would adding a job and removing it when finished ever be thread safe?
The robust solution is to use a ‘condition variable’. These allow you to suspend a thread such that it consumes zero CPU until you notify it that it has some work to do, then it wakes up, processes any items in the queue and goes back to a suspended state. Using a condition variable with a mutex like this avoids all race conditions.
These are the variables you need:
// Background communication thread.
std::thread background;
std::mutex backgroundMutex;
std::condition_variable backgroundSignal;
std::atomic<bool> killBackgroundthread = {};
to exit the queue, use the atomic flag, your loop looks something like:
while(!killBackgroundthread)
{
while (queue.empty())
processQueue();
// suspend until we're signalled.
{
std::unique_lock<std::mutex> lk(backgroundMutex);
backgroundSignal.wait(lk);
}
}
to signal the queue to wake up:
{
std::unique_lock<std::mutex> lk(backgroundMutex);
backgroundSignal.notify_one();
}Don’t you still have the same problem where a job could potentially be added between the end of the while loop and where you acquire the lock?
1 Like
the lock ensures that you can’t signal until the background thread is waiting. i.e. you can’t signal while it’s in the “while(queue.empty())” part.
so, provided the foreground thread places things on the queue before signaling the background thread to wake up, it can’t suffer a race condition.
I the queue itself would also need to be safe from race-conditions.
I see data races in your code, as you are accessing queue.empty() while other threads could append jobs to queue.
I don’t see much gain in starting and stopping threads all the time (it’s a costly operation), depending on the frequency of addJob calls, you might get less kernel calls and better performance in spawning it once and idle it on a condition variable.
Brings us back to ThreadPool 
Well a single thread with a processing queue is a threadpool afterall, i don’t see what you are trying to optimise in the first place
You don’t need to lock when notify_one:
The notifying thread does not need to hold the lock on the same mutex as the one held by the waiting thread(s); in fact doing so is a pessimization, since the notified thread would immediately block again, waiting for the notifying thread to release the lock.
From c++ standard.
In any case you are not locking accesses to queue, which is the most important part.
Also adding jobs from user mouse clicks, which could be starting and stopping threads undeterministically and even too often than needed is a really poor design choice
In case it’s useful data:
The cost of creating a thread: Apparently creating a thread takes less than 100µs* and much less on Linux. On OSX maintaining it probably requires 6kB of data. It’s really not that expensive relative to the network operation we are going to be doing on the thread.
And in our use case the thread will not be required by most users most of the time, some will never use it.
I’m interested in this problem both academically as I don’t know what the best solution to it as a logic puzzle is, but also practically as it crops up a bit for us and we have several incomplete solutions in place.
We did start using a shared ThreadPool for some cases, which would be better if we could have it manage job priority and also if we had a simple way of discarding Jobs that complete after their creator has been deleted. Those issues are complex and I haven’t had time to work on it.
Sources :-
- Thread Management OSX Data
- c++ - How much overhead is there when creating a thread? - Stack Overflow Windows data in a comment
I think:
If your thread has reached the end of the run function but not returned from it yet then it gets interrupted and control is passed to the main thread, which add a job and calls startThread(…) then the thread will not start and the queue will have a single job waiting.
I was thinking the background thread calls the run of your download job and returns. My thinking was the checking if to stop the thread would happen here, on the background thread.
If it was the main thread it wouldn’t be possible without a timer or AsyncUpdater, or did I miss something?
I achieved that by having the creator owning the job in a unique_ptr. In the destructor I call the removeJob(), that way there are no dangling jobs. Worked quite straight forward.
It just doesn’t look as fancy as the addJob (std::function) and the other deleteJobWhenFinished solution.
I think a timer as a backstop against the race condition is probably the sensible way around all this.
This solution is good, but the jobs we had tend to be network jobs and can’t be interrupted quickly reliably. So to avoid the UI blocking we either need to use our lazy deleter on the calling object, or allow the job to be independent of the UI object, and that precludes the direct unique_ptr solution you have.
I see, that makes sense. The lazy deleter sounds like a great helper, but I can imagine one would want to avoid that.
In my case the job is on the local file system, so stopping is easier, and it doesn’t happen that often (when the user deletes a clip from the timeline). When stopping the job halts the execution, that is indeed not an option.
Good luck
As a side note: We moved all our networking from the native OSX/Win32 network stuff to libcurl which has helped a lot with interrupting things, and also general reliability and supportability. But as far as I know we still can’t interrupt a network transfer instantly 
I would be fully supportive of JUCE moving to LibCurl permanently though. We had so many issues with SSL on different Windows installations!!