fix a deadlock with jack2 when inserting a plugin adds ports.

When adding a processor, the processor may add ports leading to
a call to jack_port_register(). while Ardour holds a WritertLock on the
processor-list (this commit removes this WriterLock).

with jack2 that results in a graph-reorder callback (WHY?)

jack2 issues that graph-reorder in a separate thread BUT
port-registration does not return until the graph-reorder is complete.

On Ardour's side, graph_reordered() calls Session::resort_routes ()
which eventually checks Route::direct_feeds_according_to_reality()
which needs a ReadLock on the processor-list to check I/O.

Since jack_port_register() does not return, this constitutes a deadlock.

THE ACTUAL PROBLEM IS JACK2's THREAD DESIGN!
Why does jack_port_register() trigger a graph-order in jack2?
No connections are made.
..and why does it block jack_port_register() from returning if
that graph-order callback is in a different thread?
http://pastebin.com/DZANXJLz

diff --git a/libs/ardour/ardour/route.h b/libs/ardour/ardour/route.h
index 66a83cf67d83b64c1f9d9723321d3daa85cbbcc5..bac33ae7a147f622dc39d86580e6a9eac8e27102 100644 (file)
--- a/libs/ardour/ardour/route.h
+++ b/libs/ardour/ardour/route.h
@@ -848,7 +848,7 @@ class LIBARDOUR_API Route : public SessionObject, public Automatable, public Rou
        bool _initial_io_setup;
        bool _in_sidechain_setup;
 
-       int configure_processors_unlocked (ProcessorStreams*);
+       int configure_processors_unlocked (ProcessorStreams*, Glib::Threads::RWLock::WriterLock*);
        bool set_meter_point_unlocked ();
        void apply_processor_order (const ProcessorList& new_order);
 

diff --git a/libs/ardour/route.cc b/libs/ardour/route.cc
index 5bbb9d3562e29ab93bcb317ca90b806a94066e11..0e0113498689c7c6230f1d213f7d587054d8befa 100644 (file)
--- a/libs/ardour/route.cc
+++ b/libs/ardour/route.cc
@@ -1310,9 +1310,9 @@ Route::add_processor (boost::shared_ptr<Processor> processor, boost::shared_ptr<
                // configure redirect ports properly, etc.
 
                {
-                       if (configure_processors_unlocked (err)) {
+                       if (configure_processors_unlocked (err, &lm)) {
                                pstate.restore ();
-                               configure_processors_unlocked (0); // it worked before we tried to add it ...
+                               configure_processors_unlocked (0, &lm); // it worked before we tried to add it ...
                                return -1;
                        }
                }
@@ -1495,9 +1495,9 @@ Route::add_processors (const ProcessorList& others, boost::shared_ptr<Processor>
 
                        /* Think: does this really need to be called for every processor in the loop? */
                        {
-                               if (configure_processors_unlocked (err)) {
+                               if (configure_processors_unlocked (err, &lm)) {
                                        pstate.restore ();
-                                       configure_processors_unlocked (0); // it worked before we tried to add it ...
+                                       configure_processors_unlocked (0, &lm); // it worked before we tried to add it ...
                                        return -1;
                                }
                        }
@@ -1715,7 +1715,7 @@ Route::clear_processors (Placement p)
                }
 
                _processors = new_list;
-               configure_processors_unlocked (&err); // this can't fail
+               configure_processors_unlocked (&err, &lm); // this can't fail
        }
 
        processor_max_streams.reset();
@@ -1815,10 +1815,10 @@ Route::remove_processor (boost::shared_ptr<Processor> processor, ProcessorStream
                        return 1;
                }
 
-               if (configure_processors_unlocked (err)) {
+               if (configure_processors_unlocked (err, &lm)) {
                        pstate.restore ();
                        /* we know this will work, because it worked before :) */
-                       configure_processors_unlocked (0);
+                       configure_processors_unlocked (0, &lm);
                        return -1;
                }
 
@@ -1912,9 +1912,9 @@ Route::replace_processor (boost::shared_ptr<Processor> old, boost::shared_ptr<Pr
                        }
                }
 
-               if (configure_processors_unlocked (err)) {
+               if (configure_processors_unlocked (err, &lm)) {
                        pstate.restore ();
-                       configure_processors_unlocked (0);
+                       configure_processors_unlocked (0, &lm);
                        return -1;
                }
 
@@ -2009,10 +2009,10 @@ Route::remove_processors (const ProcessorList& to_be_deleted, ProcessorStreams*
 
                _output->set_user_latency (0);
 
-               if (configure_processors_unlocked (err)) {
+               if (configure_processors_unlocked (err, &lm)) {
                        pstate.restore ();
                        /* we know this will work, because it worked before :) */
-                       configure_processors_unlocked (0);
+                       configure_processors_unlocked (0, &lm);
                        return -1;
                }
                //lx.unlock();
@@ -2063,7 +2063,7 @@ Route::configure_processors (ProcessorStreams* err)
 
        if (!_in_configure_processors) {
                Glib::Threads::RWLock::WriterLock lm (_processor_lock);
-               return configure_processors_unlocked (err);
+               return configure_processors_unlocked (err, &lm);
        }
 
        return 0;
@@ -2179,7 +2179,7 @@ Route::try_configure_processors_unlocked (ChanCount in, ProcessorStreams* err)
  *  Return 0 on success, otherwise configuration is impossible.
  */
 int
-Route::configure_processors_unlocked (ProcessorStreams* err)
+Route::configure_processors_unlocked (ProcessorStreams* err, Glib::Threads::RWLock::WriterLock* lm)
 {
 #ifndef PLATFORM_WINDOWS
        assert (!AudioEngine::instance()->process_lock().trylock());
@@ -2206,12 +2206,37 @@ Route::configure_processors_unlocked (ProcessorStreams* err)
        processor_out_streams = _input->n_ports();
        processor_max_streams.reset();
 
+       /* processor configure_io() may result in adding ports
+        * e.g. Delivery::configure_io -> ARDOUR::IO::ensure_io ()
+        *
+        * with jack2 adding ports results in a graph-order callback,
+        * which calls Session::resort_routes() and eventually
+        * Route::direct_feeds_according_to_reality()
+        * which takes a ReaderLock (_processor_lock).
+        *
+        * so we can't hold a WriterLock here until jack2 threading
+        * is fixed.
+        *
+        * NB. we still hold the process lock
+        *
+        * (ardour's own engines do call graph-order from the
+        * process-thread and hence do not have this issue; besides
+        * merely adding ports won't trigger a graph-order, only
+        * making connections does)
+        */
+       lm->release ();
+
+       // TODO check for a potential ReaderLock after ReaderLock ??
+       Glib::Threads::RWLock::ReaderLock lr (_processor_lock);
+
        list< pair<ChanCount,ChanCount> >::iterator c = configuration.begin();
        for (ProcessorList::iterator p = _processors.begin(); p != _processors.end(); ++p, ++c) {
 
                if (!(*p)->configure_io(c->first, c->second)) {
                        DEBUG_TRACE (DEBUG::Processors, string_compose ("%1: configuration failed\n", _name));
                        _in_configure_processors = false;
+                       lr.release ();
+                       lm->acquire ();
                        return -1;
                }
                processor_max_streams = ChanCount::max(processor_max_streams, c->first);
@@ -2245,6 +2270,9 @@ Route::configure_processors_unlocked (ProcessorStreams* err)
                }
        }
 
+       lr.release ();
+       lm->acquire ();
+
 
        if (_meter) {
                _meter->set_max_channels (processor_max_streams);
@@ -2438,7 +2466,7 @@ Route::reorder_processors (const ProcessorList& new_order, ProcessorStreams* err
 
                apply_processor_order (new_order);
 
-               if (configure_processors_unlocked (err)) {
+               if (configure_processors_unlocked (err, &lm)) {
                        pstate.restore ();
                        return -1;
                }
@@ -2511,7 +2539,7 @@ Route::add_remove_sidechain (boost::shared_ptr<Processor> proc, bool add)
                        return false;
                }
                lx.acquire ();
-               configure_processors_unlocked (0);
+               configure_processors_unlocked (0, &lm);
        }
 
        if (pi->has_sidechain ()) {
@@ -2554,7 +2582,7 @@ Route::plugin_preset_output (boost::shared_ptr<Processor> proc, ChanCount outs)
                        pi->set_preset_out (old);
                        return false;
                }
-               configure_processors_unlocked (0);
+               configure_processors_unlocked (0, &lm);
        }
 
        processors_changed (RouteProcessorChange ()); /* EMIT SIGNAL */
@@ -2614,7 +2642,7 @@ Route::customize_plugin_insert (boost::shared_ptr<Processor> proc, uint32_t coun
 
                        return false;
                }
-               configure_processors_unlocked (0);
+               configure_processors_unlocked (0, &lm);
        }
 
        processors_changed (RouteProcessorChange ()); /* EMIT SIGNAL */
@@ -3426,7 +3454,7 @@ Route::set_processor_state (const XMLNode& node)
                _processors = new_order;
 
                if (must_configure) {
-                       configure_processors_unlocked (0);
+                       configure_processors_unlocked (0, &lm);
                }
 
                for (ProcessorList::const_iterator i = _processors.begin(); i != _processors.end(); ++i) {
@@ -3746,8 +3774,8 @@ Route::direct_feeds_according_to_reality (boost::shared_ptr<Route> other, bool*
                return true;
        }
 
+       Glib::Threads::RWLock::ReaderLock lm (_processor_lock);
 
-       Glib::Threads::RWLock::ReaderLock lm (_processor_lock); // XXX
        for (ProcessorList::iterator r = _processors.begin(); r != _processors.end(); ++r) {
 
                boost::shared_ptr<IOProcessor> iop = boost::dynamic_pointer_cast<IOProcessor>(*r);
@@ -4271,10 +4299,10 @@ Route::listen_position_changed ()
                Glib::Threads::RWLock::WriterLock lm (_processor_lock);
                ProcessorState pstate (this);
 
-               if (configure_processors_unlocked (0)) {
+               if (configure_processors_unlocked (0, &lm)) {
                        DEBUG_TRACE (DEBUG::Processors, "---- CONFIGURATION FAILED.\n");
                        pstate.restore ();
-                       configure_processors_unlocked (0); // it worked before we tried to add it ...
+                       configure_processors_unlocked (0, &lm); // it worked before we tried to add it ...
                        return;
                }
        }
@@ -4295,7 +4323,7 @@ Route::add_export_point()
                _capturing_processor.reset (new CapturingProcessor (_session));
                _capturing_processor->activate ();
 
-               configure_processors_unlocked (0);
+               configure_processors_unlocked (0, &lw);
 
        }