Fixed minor race condition and priority inversion

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

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

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; }

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.

…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.

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).

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

Yes, (see my edit)

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

2 Likes

LGTM ! Thanks

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

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

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).

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…

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

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.

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

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…

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

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