Fixed minor race condition and priority inversion

Development
1

Hello,

I think there is a potential race condition in AudioProcessor::createEditorIfNeeded if called from several threads : multiple editors could be created.

The fix would be to take the lock earlier in the method.

Also, it can be a dedicated CriticalSection imho (or does it absolutely need to be the callback CriticalSection?), so in the fix I proposed hereunder I add a CriticalSection dedicated to the protection of the access to activeEditor member. This will also prevent priority inversion problems (where creating the editor could block audio processing)

Here is the fix I propose, in my fork:

Have a good day!
Olivier

2 Likes
2

Have you actually seen this happen in practice? AFAIK none of the plug-in format wrappers call createEditorIfNeeded() concurrently and will only call it on the message thread.

The issue with your proposed changes is that there is now a more serious priority inversion which can occur if, for example, getActiveEditor() is called in processBlock as at the moment this is guaranteed to be fast and lock-free but with your changes this may block whilst the editor is being created.

1 Like
3

Have you actually seen this happen in practice?

No, but better safe than sorry later if things changeā€¦

Also, before my changes, createEditorIfNeeded took the callback lock, which can result in a priority inversion and audio dropout

The issue with your proposed changes is that there is now a more serious priority inversion which can occur if, for example, getActiveEditor() is called in processBlock

No, because AFAIK, getActiveEditor doesnā€™t lock :

AudioProcessorEditor* getActiveEditor() const noexcept { return activeEditor; }

4

Which is the problem. All the effort guarding the activeEditor was in vain, if it is returned here without a lock.

I was wondering if wrapping the pointer into std::atomic would help, but it also just shifts the problem somewhere else. If construction and deletion happen on arbitrary threads, you are in hells kitchen already IMHO.

5

ā€¦which is never safe to do, since the activeEditor can be deleted on the message thread. There is no safe way to call into the editor from processBlock, the locking doesnā€™t change that.

6

In the current version, just before actually deleting the editor, editorBeingDeleted is called, and it takes the callback lock, so it is actually safe to use getActiveEditor in processBlock (because the caller of processBlock also owns the callback lock). The downside is that it can create a priority inversion.

In my version, since I was thinking no-one would actually call getActiveEditor in the audio processing thread (it sounds like wanting to do UI stuff in the realtime audio thread, which is not ideal), it becomes unsafe to do that. But the upside is that there is no more priority inversion when the editor is created or deleted (because instead of taking the callback lock, editorBeingDeleted and createEditorIfNeeed take a dedicated lock).

7

But your PR replaced the guard with the additional editorLock which is unrelated to the callbackLock, or did I understand that wrong?

8

Yes, (see my edit)

9

Thanks for the input. Iā€™ve pushed some changes addressing this and updated the documentation for getActiveEditor() here:

2 Likes
10

LGTM ! Thanks

11

I guess one way to have more safety around this would be to wrap the pointer in a std::shared_ptr (and use the lock to update / copy it, until we have std::atomic_shared_ptr in c++20 ) and getActiveEditor would then return a (copy of the) shared_ptr

1 Like
12

std::atomic_shared_ptr will be the same as std::atomic_load/store

13

I donā€™t think that is a good idea. shared_ptr moves the destruction of the object to an almost uncontrollable place, so it could happen on the audio thread or any other thread (and the destruction of components is only safe on the message thread).

14

I didnā€™t know that.

Another solution, to allow ā€œsafeā€ usage of the activeEditor on another (UI) thread than the message thread would be to make the lock recursive and provide a method in the api so that the user can take the lock as long as it will use the pointer returned by getActiveEditorā€¦ but thatā€™s a bit convolutedā€¦

15

Iā€™m still trying to find a way to reproduce this / regress this in a simple example but in the meantime I must say that it would be also wise to add this as breaking change.

A rationale of knowing (even if itā€™s not thread safe) if thereā€™s a editor is for some calculations youā€™d like to do only when thereā€™s UI.

In our products thereā€™s a code that do things like spectrum analyzers / meters / etc / notify on preset changes / etc only when thereā€™s active editor.

You wrote yourself (@ed95) :

The issue with your proposed changes is that there is now a more serious priority inversion which can occur if, for example, getActiveEditor() is called in processBlock as at the moment this is guaranteed to be fast and lock-free but with your changes this may block whilst the editor is being created.

@daniel:

ā€¦which is never safe to do, since the activeEditor can be deleted on the message thread. There is no safe way to call into the editor from processBlock, the locking doesnā€™t change that.

Indeed, but the lock now makes it impossible for the processor to even ā€˜just knowā€™ if thereā€™s activeEditor without risk of locking on audio thread.
I still donā€™t see the advantage of the lock on getActiveEditor().

Hereā€™s an example:

    if (AudioProcessorEditor* editor = processor.getActiveEditor())
    {
        processor.editorBeingDeleted (editor);
        delete editor;
    }

this is inside the AUv3 wrapper.
so if itā€™s assumed to be called only by the message thread than the entire call already is on the message thread.
and both getActiveEditor and editorBeingDelete (and createActiveEditorā€¦) has locks.
But even if we had other threads,
getActiveEditor() could return something (with lock) that will be invalid the second after.

1 Like
16

I think itā€™s about createEditorIfNeeded, which may be called from multiple threads -thatā€™s the point of the fix. Anyway I donā€™t see the advantage either -in the worst case getActiveEditor would return nullptr while the editor is being created, which doesnā€™t seem that terrible.

17

If this case wouldnā€™t it make more sense to just lock for createEditorIfNeeded, thus not in getActiveEditor so not breaking the ability to check if thereā€™s a GUI to make calculations for?

2 Likes
18

Iā€™ve made an example that show how getActiveEditor() can easily create stutter with Ableton Live 10.1 and VST3 under macOS.

Code side:

  1. create a new projucer Audio Plug-In template.
  2. in the AudioProcessor::processBlock():
    if (getActiveEditor() != nullptr)
    {
        auto mag = buffer.getMagnitude (0, buffer.getNumSamples());
    }
  1. in the AudioProcessorEditor constructor:
    sleep(1); // our plug-in for example takes enough time to construct. this simulates this.

The Ableton project:

  1. create new project.
  2. add an audio clip (or any audio signal)
  3. drag the VST3 plug-in on the track.
  4. open/close editor a few timesā€¦
19

Surely you shouldnā€™t be calling getActiveEditor() from the audio thread?
I know thereā€™s a recently added comment to clarify this:

        Note that you should only call this method from the message thread as the active
        editor may be deleted by the message thread, causing a dangling pointer.

But even before this was added, calling a method that returns a pointer to an object which we know is managed by a different thread feels like a recipe for disaster.

Wouldnā€™t a better approach be to simply have any editors increment an atomic which you can use to know if there is an editor?

An alternative would be to add a method to AudioProcessor like hasActiveEditor() which could attempt a try-lock and if the lock can not be taken or it can and activeEditor is not-null then return true.

This sounds like extra bloat to me though and I know youā€™re trying to avoid having to change existing code because the old behaviour didnā€™t block.

1 Like
20

It sounds like youā€™re thinking about it the wrong way round.

Instead of thinking ā€œthe audio thread should check whether thereā€™s an editor, so it can decide whether to do some work or notā€ try rephrasing the task as ā€œthe editor should tell the audio thread what it needs to doā€.

So e.g. if you only want to calculate the magnitude when an editor needs it, have a std::atomic<bool> shouldCalculateMagnitude; and make sure an editor set it to true when itā€™s showing, and false when itā€™s hidden.

1 Like