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.

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

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…