COCKOS
CONFEDERATED FORUMS
Cockos : REAPER : NINJAM : Forums
Forum Home : Register : FAQ : Members List : Search :
Old 09-03-2018, 09:03 AM   #1
pressplay
Human being with feelings
 
pressplay's Avatar
 
Join Date: Sep 2017
Location: Berlin
Posts: 47
Default Concurrency issues in GUI thread

Hi there,


Please help me with the following. I have a class SignalDisplay inheriting from IControl and this class has a sample container in it. The audio thread writes samples into this container (downsampled), while the Draw method of the SignalDisplay reads the samples. Occasional crashes confirm that this is not thread safe.


I have solved this by locking a mutex like this:


Code:
bool SignalDisplay::Draw(IGraphics *pGraphics)
{
    IPlugBase::IMutexLock(pGraphics->GetPlug());
    // more code...
}
Please correct me if I'm totally wrong? Are there any better options?


Thank you!
pressplay is offline   Reply With Quote
Old 09-03-2018, 10:58 AM   #2
olilarkin
Human being with feelings
 
Join Date: Apr 2009
Location: Berlin, Germany
Posts: 1,248
Default

this is seriously wrong and will cause you pain. IPlug1 is already over-compensating for thread safety, but locking in the Draw() method like this is going to increase chances of problems. If you can describe with code, what you were doing before locking this mutex, that might make it clearer why you had a crash.
__________________
VirtualCZ | Endless Series | iPlug2 | Linkedin | Facebook
olilarkin is offline   Reply With Quote
Old 09-03-2018, 12:01 PM   #3
pressplay
Human being with feelings
 
pressplay's Avatar
 
Join Date: Sep 2017
Location: Berlin
Posts: 47
Default

Okay, I had a bad feeling about this, that's why I was asking.

SignalDisplay is attached to the graphics object in IPlug constructor as usual. It has a std:deque member m_displaySamples initialized to the correct size. And it has a function that is called from the audio thread:
Code:
void SignalDisplay::addSamples(const std::vector<double> &f_samples)
{
    for (const auto& sample : f_samples) {
        m_displaySamples.push_back(sample);
        m_displaySamples.pop_front();
    }


    // more things here...
}
Then in the Draw function, samples are accessed in a for loop by means of the [] operator. The index is not getting out of bounds.

I'm getting a debug assertion on Windows:
Code:
Expression: deque iterator not dereferencable.
Update: Not occasional, everytime.

In release mode, everything runs smoothly, but I have no trust anymore in what I am doing. Visual Studio 2015 (v140) if this helps.
pressplay is offline   Reply With Quote
Old 09-03-2018, 01:18 PM   #4
Xenakios
Human being with feelings
 
Xenakios's Avatar
 
Join Date: Feb 2007
Location: Oulu, Finland
Posts: 8,062
Default

Quote:
Originally Posted by pressplay View Post

I'm getting a debug assertion on Windows:
Code:
Expression: deque iterator not dereferencable.
Update: Not occasional, everytime.

In release mode, everything runs smoothly, but I have no trust anymore in what I am doing. Visual Studio 2015 (v140) if this helps.
The std:: containers are not thread safe, so you will have to solve the thread safety issue in some way. The reason you are not getting a crash with release builds is that the behavior is going to be undefined, including the code working 99.99% of the time and then crashing or corrupting data at the most inconvenient moment. With debug builds the Microsoft C++ standard library does various checks with the containers. (But even those might not catch thread safety issues consistently, you have just been lucky for now...)
__________________
I am no longer part of the REAPER community. Please don't contact me with any REAPER-related issues.
Xenakios is offline   Reply With Quote
Old 09-04-2018, 12:16 AM   #5
pressplay
Human being with feelings
 
pressplay's Avatar
 
Join Date: Sep 2017
Location: Berlin
Posts: 47
Default

Combining the two answers: IPlug is already thread safe, but std containers are not. Locking a mutex like I did is "seriously wrong", but I will have to solve the issue "in some way". Maximum confusion


As I only write into the container within the audio thread (mutex locked) and read from another thread using the const [] operator, I don't see any races happening. Apperently, some iterators are invalidated while reading. Why? And does it really matter?
pressplay is offline   Reply With Quote
Old 09-04-2018, 12:51 AM   #6
pressplay
Human being with feelings
 
pressplay's Avatar
 
Join Date: Sep 2017
Location: Berlin
Posts: 47
Default

Solved temporarily by adding a mutex to the SignalDisplay class and locking on read and write. But I don't think this is really necessary, as IPlugBase already has a mutex object.
pressplay is offline   Reply With Quote
Old 09-04-2018, 03:02 AM   #7
Xenakios
Human being with feelings
 
Xenakios's Avatar
 
Join Date: Feb 2007
Location: Oulu, Finland
Posts: 8,062
Default

Quote:
Originally Posted by pressplay View Post
Maximum confusion
It's always that when multithreading is involved.

Quote:
Originally Posted by pressplay View Post
As I only write into the container within the audio thread (mutex locked) and read from another thread using the const [] operator, I don't see any races happening. Apperently, some iterators are invalidated while reading. Why? And does it really matter?
std::deque is a complicated container which you probably shouldn't even be using in audio code, as it for example can reallocate when adding data to it. While it reallocates or changes its other internal state, it will be in an indeterminate state and concurrently reading from it is not allowed. The read operation being "const" does not help here. If you have seen the slogan "since C++11 const means thread safe", that obviously applies only while every thread is using a const method of an object. So it's not going to work here because you are concurrently using a non-const method from the audio thread.

The issue is serious, the code will eventually crash or bug out in some other manner with release builds if you don't access the container thread safely. And even if it never apparently does, it's still technically wrong. You have just been lucky to catch the problem with the Windows debug builds because the Microsoft std::deque implementation does some internal state checks.

Using a mutex (the same mutex instance obviously) during the write and read operations is going to work for making it thread safe, but it's not ideal for audio code, which is what Oli said. It's just not a good idea to make the audio thread potentially wait for something expensive happening in the GUI thread, in this case the drawing of the control. You should attempt to make things work so that the mutex lock only needs to be held for a very brief period of time. In an ideal situation, one wouldn't be using mutex locks at all, but that is often very complicated to implement and can just cause worse problems than using mutexes if one doesn't implement things exactly right.
__________________
I am no longer part of the REAPER community. Please don't contact me with any REAPER-related issues.

Last edited by Xenakios; 09-04-2018 at 03:19 AM.
Xenakios is offline   Reply With Quote
Old 09-04-2018, 03:16 AM   #8
pressplay
Human being with feelings
 
pressplay's Avatar
 
Join Date: Sep 2017
Location: Berlin
Posts: 47
Default

Many thanks for your help (and thank you Microsoft...). I don't use this for audio, only for displaying stuff. The operations are computationally negligible, so my lock guard solution seems okay for now.
pressplay is offline   Reply With Quote
Old 09-04-2018, 03:22 AM   #9
Xenakios
Human being with feelings
 
Xenakios's Avatar
 
Join Date: Feb 2007
Location: Oulu, Finland
Posts: 8,062
Default

Quote:
Originally Posted by pressplay View Post
I don't use this for audio, only for displaying stuff.
It may still interfere with other audio processing in the host because the audio callbacks get stalled waiting for your GUI to draw...
__________________
I am no longer part of the REAPER community. Please don't contact me with any REAPER-related issues.
Xenakios is offline   Reply With Quote
Old 09-04-2018, 03:41 AM   #10
pressplay
Human being with feelings
 
pressplay's Avatar
 
Join Date: Sep 2017
Location: Berlin
Posts: 47
Default

True. My assumption was that pushing a handful of samples from time to time and reading let's say a few hundreds in the Gui thread won't block my audio thread that much.
So the next question would be: How to do it properly if you want to store data which is required by the IControl object for drawing. This seems like a very common task.
pressplay is offline   Reply With Quote
Old 09-04-2018, 03:46 AM   #11
Xenakios
Human being with feelings
 
Xenakios's Avatar
 
Join Date: Feb 2007
Location: Oulu, Finland
Posts: 8,062
Default

Quote:
Originally Posted by pressplay View Post
My assumption was that pushing a handful of samples from time to time and reading let's say a few hundreds in the Gui thread won't block my audio thread that much.
It maybe wouldn't be too bad if you just did some sample data copying, but at least in the code you initially posted, you put the mutex locker at the top of the Draw function, which would make the lock also be held during the drawing operations.
__________________
I am no longer part of the REAPER community. Please don't contact me with any REAPER-related issues.
Xenakios is offline   Reply With Quote
Old 09-04-2018, 04:01 AM   #12
pressplay
Human being with feelings
 
pressplay's Avatar
 
Join Date: Sep 2017
Location: Berlin
Posts: 47
Default

So instead of this
Code:
bool SignalDisplay::Draw(IGraphics * pGraphics)
{ // pseudo code
    for each sample {
        lock guard;
        pGraphics->drawStuff(sample);
    }
    ...
}
you would suggest to do it like this?
Code:
bool SignalDisplay::Draw(IGraphics * pGraphics)
{ // pseudo code
    {
        lock guard;
        local_samples = copy of samples;
    }
    for each local_sample {
        pGraphics->drawStuff(local_sample);
    }
    ...
}
pressplay is offline   Reply With Quote
Old 09-04-2018, 04:06 AM   #13
Xenakios
Human being with feelings
 
Xenakios's Avatar
 
Join Date: Feb 2007
Location: Oulu, Finland
Posts: 8,062
Default

Quote:
Originally Posted by pressplay View Post
you would suggest to do it like this?
Code:
bool SignalDisplay::Draw(IGraphics * pGraphics)
{ // pseudo code
    {
        lock guard;
        local_samples = copy of samples;
    }
    for each local_sample {
        pGraphics->drawStuff(local_sample);
    }
    ...
}
Yeah, something along those lines. (Of course the "local samples" should be something that is preallocated to have enough space for the samples so that no reallocation takes place.)
__________________
I am no longer part of the REAPER community. Please don't contact me with any REAPER-related issues.
Xenakios is offline   Reply With Quote
Old 02-20-2019, 12:32 PM   #14
pressplay
Human being with feelings
 
pressplay's Avatar
 
Join Date: Sep 2017
Location: Berlin
Posts: 47
Default

Just came back to this piece of code. Now I understand why it was so wrong and I accept the "you shall never lock the audio thread" rule. It turns out that the problem could be solved lock-free, with less lines of code and less copying Of course it requires more cpp knowledge that I had at that time.
pressplay is offline   Reply With Quote
Reply

Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off

Forum Jump


All times are GMT -7. The time now is 01:05 AM.


Powered by vBulletin® Version 3.8.11
Copyright ©2000 - 2024, vBulletin Solutions Inc.