Ableton Live deadlock in plugin with AudioProcessorGraph

Our plugin contains an AudioProcessorGraph and the VST won’t load in Ableton Live 8 or 9 (the AU is fine), I can recreate the same problem in the demo plugin by adding this to PluginProcessor.h :

AudioProcessorGraph graph;

and this to the end of JuceDemoPluginAudioProcessor::prepareToPlay :

graph.prepareToPlay( sampleRate, samplesPerBlock );

What is happening is that just after the plugin is loaded the graph’s prepare to play is trying to lock the message thread, and waiting forever. Eventually Live gives up and pops up a window saying “Fatal error: the audio engine is not responding”.

At the time the plugin is doing this:

#0 0x95183aa2 in __semwait_signal
#1 0x9518375e in _pthread_cond_wait
#2 0x951832b1 in pthread_cond_timedwait$UNIX2003
#3 0x286f1658 in juce::WaitableEventImpl::wait at juce_posix_SharedCode.h:121
#4 0x28689938 in juce::WaitableEvent::wait at juce_posix_SharedCode.h:174
#5 0x2872dc93 in juce::MessageManagerLock::attemptLock at juce_MessageManager.cpp:287
#6 0x2872deb8 in juce::MessageManagerLock::MessageManagerLock at juce_MessageManager.cpp:249
#7 0x286306bc in juce::AudioProcessorGraph::buildRenderingSequence at juce_AudioProcessorGraph.cpp:1208
#8 0x28630cf3 in juce::AudioProcessorGraph::prepareToPlay at juce_AudioProcessorGraph.cpp:1272
#9 0x2850f28d in JuceDemoPluginAudioProcessor::prepareToPlay at PluginProcessor.cpp:217
#10 0x28a6dfb9 in JuceVSTWrapper::resume at juce_VST_Wrapper.cpp:649
#11 0x28a65768 in AudioEffect::dispatcher at audioeffect.cpp:167
#12 0x28a66b0b in AudioEffectX::dispatcher at audioeffectx.cpp:297
#13 0x28a6fb46 in JuceVSTWrapper::dispatcher at juce_VST_Wrapper.cpp:1181
#14 0x28a65389 in AudioEffect::dispatchEffectClass at audioeffect.cpp:32

Has anyone else come across this? Have a solution?

The messageManagerLock should be replace by a regular CriticalSection-lock.
The current implementation isn’t suitable for plugins.

Thanks, I switched to a CriticalSection to prevent re-entry into buildRenderingSequence() which I believe is the purpose of the current locking. It seems to work.

I also added a getBuildLock() function because I need to make sure the build is locked when setting a plugin’s state to avoid crashes with some plugins.

Here are the changes in case anyone else wants them:

===================================================================
— juce/modules/juce_audio_processors/processors/juce_AudioProcessorGraph.cpp (revision 2191)
+++ juce/modules/juce_audio_processors/processors/juce_AudioProcessorGraph.cpp (working copy)
@@ -1205,8 +1205,7 @@
int numMidiBuffersNeeded = 1;

 {
  •    MessageManagerLock mml;
    
  •    const ScopedLock sl (buildLock);
       Array<void*> orderedNodes;
    
       {
    

Index: juce/modules/juce_audio_processors/processors/juce_AudioProcessorGraph.h

— juce/modules/juce_audio_processors/processors/juce_AudioProcessorGraph.h (revision 2191)
+++ juce/modules/juce_audio_processors/processors/juce_AudioProcessorGraph.h (working copy)
@@ -248,6 +248,15 @@
*/
bool removeIllegalConnections();

  • /** Returns a lock that prevents the rendering sequence being built
  •    This is useful when the graph contains plugins that run the message loop
    
  •    during operations (for example GuitarRig and setStateInformation) which could result
    
  •    in the rendering sequence being rebuilt which results in the other
    
  •    calls being made to the plugin which can crash it.
    
  • */
  • inline const CriticalSection& getBuildLock() const noexcept { return buildLock; }
  • //==============================================================================
    /** A special number that represents the midi channel of a node.

@@ -412,6 +421,8 @@
void buildRenderingSequence();
bool isAnInputTo (uint32 possibleInputId, uint32 possibleDestinationId, int recursionCheck) const;

  • CriticalSection buildLock;
  • JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (AudioProcessorGraph)
    };

if jules could implement these changes, this would be cool. And if he reading this, it would be nice if also add this few simple changes i mention here ( http://rawmaterialsoftware.com/viewtopic.php?f=2&t=10926&hilit=audioprocessorgraph#p61834 too.

No… the reason it used an MM lock was because it expects the connection to be added and removed by the message thread (without any other locking), so it’s to prevent connections being changed while it’s building the sequence.

True, when I wrote it, I didn’t design the class to be used in a plugin, but yes, it could be changed to use a criticalsection. It would however need more careful thought than just swapping the MM lock for a CriticalSection, and all the other methods would also need to be locked.

Got exactly the same problem.

What would be the best way to replace the MessageManagerLock in the current JUCE version?

thanks

oli

bump - Jules is there any possibility this can be changed on the library side to make AudioProcessorGraph more plug-in friendly?

thanks

oli

It's really a question of making it more multithreading-friendly rather than plugin-friendly. We don't have bandwidth to look at it right now, it might be easy to change to a criticalsection, or it might be a hydra, hard to tell! TBH I'd suggest a good short-term fix would be to use an async event to do these changes on the message thread rather than trying to do them from a background thread.

I have two questions:

1) Can someone help me understand why would all the other AudioProcessorGraph methods need to be locked? Perhaps you were not referring to all the other AudioProcessorGraph methods? Though I am admittedly new to reasoning about concurrency on this level, it seems to me that this approach is safe as long as buildRenderingSequence() acquires the buildLock, and any other methods that attempt to add new connections also attempt to acquire the buildLock. 

2) Why does buildLock have to be a re-entrant mutex (CriticalSection is a re-entrant mutex according to the docs) as a opposed to a non re-entrant mutex? I just read the wikipedia page on re-entrant mutexes and learned that a re-entrant mutex differers from a non-re-entrant mutex in that attempts to lock by the same thread that currently owns the lock will not block. This is useful when a method acquires a lock, then either calls itself or calls a method that, in turn, calls itself. Instead of blocking, the re-entrant mutex recognizes that the same thread that owns the lock is trying to acquire the lock, and does not block. It does not seem that buildRenderingSequence() will ever exhibit this behavior. Is the reason that CriticalSection was suggested as a lock simply because there is no downside to using a re-entrant mutex as a opposed to a "normal" mutex? 

Thanks in advance to anyone who's willing to discuss this with me. 

- Doron

Any fix?

Rail

Sorry, nothing new about this issue at the moment beyond what was already said in this thread.

This looks like a complicated issue. Unfortunately we don't quite have the time to dive into this right now as we are quite busy with finalising JUCE 4.

Maybe someone else here has the chance to have a deeper look into this?

Any word on this since juce 4?

We've not looked at it, but you could try my earlier suggestion of using an AsyncUpdater to do this on the message thread rather than the audio thread.