From 3c857a78c65cae279804ae5a210ea68b1bf006f1 Mon Sep 17 00:00:00 2001 From: Paul Davis Date: Wed, 13 Sep 2023 09:20:41 -0600 Subject: [PATCH] JACK backend: serialize all jack server calls with a mutex As was noted in 88ee3af3ea26 it is unsafe/undefined behavior if two threads sleep on the JACK request file descriptor, since there is no way to control which one will wake and process the request. Since each thread may have sent a different request, this can lead to a thread misinterpreting the response because it is reading the wrong response. This may (or may not) solve some subtle problems with JACK, but was revealed by having a control surface (LaunchPad Pro) that registers three ports from the butler thread at about the same as the GUI thread is registering the auditioner. One thread read the wrong response, and because of some slightly weird code/design, it attempts to rename the port from within the response handler, which in JACK1 leads to deadlock (and later, zombification). --- libs/backends/jack/jack_audiobackend.cc | 57 ++++++++++------- libs/backends/jack/jack_audiobackend.h | 2 +- libs/backends/jack/jack_portengine.cc | 82 +++++++++++++------------ 3 files changed, 80 insertions(+), 61 deletions(-) diff --git a/libs/backends/jack/jack_audiobackend.cc b/libs/backends/jack/jack_audiobackend.cc index 89070d146d..78e2b63bef 100644 --- a/libs/backends/jack/jack_audiobackend.cc +++ b/libs/backends/jack/jack_audiobackend.cc @@ -48,6 +48,8 @@ using std::vector; #define GET_PRIVATE_JACK_POINTER(localvar) jack_client_t* localvar = _jack_connection->jack(); if (!(localvar)) { return; } #define GET_PRIVATE_JACK_POINTER_RET(localvar,r) jack_client_t* localvar = _jack_connection->jack(); if (!(localvar)) { return r; } +// #define JACK_SERVER_CALL(expr) { std::cerr << "JACK SERVER CALL: " << pthread_self() << '/' << pthread_name() << ' ' << #expr << std::endl; Glib::Threads::Mutex::Lock lm (server_call_mutex); expr; } +#define JACK_SERVER_CALL(expr) { Glib::Threads::Mutex::Lock lm (server_call_mutex); expr; } JACKAudioBackend::JACKAudioBackend (AudioEngine& e, AudioBackendInfo& info, std::shared_ptr jc) : AudioBackend (e, info) @@ -618,7 +620,11 @@ JACKAudioBackend::freewheel (bool onoff) return 0; } - if (jack_set_freewheel (_priv_jack, onoff) == 0) { + int ret; + + JACK_SERVER_CALL (ret = jack_set_freewheel (_priv_jack, onoff)); + + if (ret == 0) { _freewheeling = onoff; return 0; } @@ -669,9 +675,9 @@ JACKAudioBackend::set_time_master (bool yn) { GET_PRIVATE_JACK_POINTER_RET (_priv_jack, -1); if (yn) { - return jack_set_timebase_callback (_priv_jack, 0, _jack_timebase_callback, this); + JACK_SERVER_CALL (return jack_set_timebase_callback (_priv_jack, 0, _jack_timebase_callback, this)); } else { - return jack_release_timebase (_priv_jack); + JACK_SERVER_CALL (return jack_release_timebase (_priv_jack)); } } @@ -741,20 +747,21 @@ JACKAudioBackend::set_jack_callbacks () * non-callback API, and run the thread init callback in our own code. */ - jack_set_process_thread (_priv_jack, _process_thread, this); - jack_set_sample_rate_callback (_priv_jack, _sample_rate_callback, this); - jack_set_buffer_size_callback (_priv_jack, _bufsize_callback, this); - jack_set_xrun_callback (_priv_jack, _xrun_callback, this); - jack_set_sync_callback (_priv_jack, _jack_sync_callback, this); - jack_set_freewheel_callback (_priv_jack, _freewheel_callback, this); + JACK_SERVER_CALL (jack_set_process_thread (_priv_jack, _process_thread, this)); + JACK_SERVER_CALL (jack_set_sample_rate_callback (_priv_jack, _sample_rate_callback, this)); + JACK_SERVER_CALL (jack_set_buffer_size_callback (_priv_jack, _bufsize_callback, this)); + JACK_SERVER_CALL (jack_set_xrun_callback (_priv_jack, _xrun_callback, this)); + JACK_SERVER_CALL (jack_set_sync_callback (_priv_jack, _jack_sync_callback, this)); + JACK_SERVER_CALL (jack_set_freewheel_callback (_priv_jack, _freewheel_callback, this)); #ifdef HAVE_JACK_SESSION - if( jack_set_session_callback) - jack_set_session_callback (_priv_jack, _session_callback, this); + if (jack_set_session_callback) { + JACK_SERVER_CALL (jack_set_session_callback (_priv_jack, _session_callback, this)); + } #endif if (jack_set_latency_callback) { - jack_set_latency_callback (_priv_jack, _latency_callback, this); + JACK_SERVER_CALL (jack_set_latency_callback (_priv_jack, _latency_callback, this)); } jack_set_error_function (ardour_jack_error); @@ -764,6 +771,7 @@ void JACKAudioBackend::_jack_timebase_callback (jack_transport_state_t state, pframes_t nframes, jack_position_t* pos, int new_position, void *arg) { + DEBUG_TRACE (DEBUG::BackendCallbacks, string_compose ("%1/%2 jack timebase callback\n", pthread_self(), pthread_name())); static_cast (arg)->jack_timebase_callback (state, nframes, pos, new_position); } @@ -782,6 +790,7 @@ JACKAudioBackend::jack_timebase_callback (jack_transport_state_t state, pframes_ int JACKAudioBackend::_jack_sync_callback (jack_transport_state_t state, jack_position_t* pos, void* arg) { + DEBUG_TRACE (DEBUG::BackendCallbacks, string_compose ("%1/%2 jack sync callback\n", pthread_self(), pthread_name())); return static_cast (arg)->jack_sync_callback (state, pos); } @@ -820,6 +829,8 @@ JACKAudioBackend::jack_sync_callback (jack_transport_state_t state, jack_positio int JACKAudioBackend::_xrun_callback (void *arg) { + DEBUG_TRACE (DEBUG::BackendCallbacks, string_compose ("%1/%2 jack sync callback\n", pthread_self(), pthread_name())); + JACKAudioBackend* jab = static_cast (arg); if (jab->available()) { jab->engine.Xrun (); /* EMIT SIGNAL */ @@ -830,6 +841,8 @@ JACKAudioBackend::_xrun_callback (void *arg) void JACKAudioBackend::_session_callback (jack_session_event_t *event, void *arg) { + DEBUG_TRACE (DEBUG::BackendCallbacks, string_compose ("%1/%2 jack session callback\n", pthread_self(), pthread_name())); + JACKAudioBackend* jab = static_cast (arg); ARDOUR::Session* session = jab->engine.session(); @@ -842,6 +855,7 @@ JACKAudioBackend::_session_callback (jack_session_event_t *event, void *arg) void JACKAudioBackend::_freewheel_callback (int onoff, void *arg) { + DEBUG_TRACE (DEBUG::BackendCallbacks, string_compose ("%1/%2 jack freewheel callback\n", pthread_self(), pthread_name())); static_cast(arg)->freewheel_callback (onoff); } @@ -855,6 +869,7 @@ JACKAudioBackend::freewheel_callback (int onoff) void JACKAudioBackend::_latency_callback (jack_latency_callback_mode_t mode, void* arg) { + DEBUG_TRACE (DEBUG::BackendCallbacks, string_compose ("%1/%2 jack latency callback\n", pthread_self(), pthread_name())); return static_cast (arg)->jack_latency_callback (mode); } @@ -882,16 +897,15 @@ JACKAudioBackend::join_process_threads () int ret = 0; - for (std::vector::const_iterator i = _jack_threads.begin (); - i != _jack_threads.end(); i++) { + for (auto & thread : _jack_threads) { #if defined(USING_JACK2_EXPANSION_OF_JACK_API) || defined __jack_systemdeps_h__ // jack_client is not used by JACK2's implementation // also jack_client_close() leaves threads active - if (jack_client_stop_thread (_priv_jack, *i) != 0) + if (jack_client_stop_thread (_priv_jack, thread) != 0) #else void* status; - if (pthread_join (*i, &status) != 0) + if (pthread_join (thread, &status) != 0) #endif { error << "AudioEngine: cannot stop process thread" << endmsg; @@ -917,15 +931,14 @@ JACKAudioBackend::in_process_thread () } #endif - for (std::vector::const_iterator i = _jack_threads.begin (); - i != _jack_threads.end(); i++) { + for (auto & thread : _jack_threads) { #if defined COMPILER_MINGW && (!defined PTW32_VERSION || defined __jack_systemdeps_h__) - if (*i == GetCurrentThread()) { + if (thread == GetCurrentThread()) { return true; } #else // pthreads - if (pthread_equal (*i, pthread_self()) != 0) { + if (pthread_equal (thread, pthread_self()) != 0) { return true; } #endif @@ -1001,6 +1014,7 @@ JACKAudioBackend::process_thread () int JACKAudioBackend::_sample_rate_callback (pframes_t nframes, void *arg) { + DEBUG_TRACE (DEBUG::BackendCallbacks, string_compose ("%1/%2 jack sample rate callback\n", pthread_self(), pthread_name())); return static_cast (arg)->jack_sample_rate_callback (nframes); } @@ -1020,6 +1034,7 @@ JACKAudioBackend::jack_latency_callback (jack_latency_callback_mode_t mode) int JACKAudioBackend::_bufsize_callback (pframes_t nframes, void *arg) { + DEBUG_TRACE (DEBUG::BackendCallbacks, string_compose ("%1/%2 jack buffer size callback\n", pthread_self(), pthread_name())); return static_cast (arg)->jack_bufsize_callback (nframes); } @@ -1086,7 +1101,7 @@ void JACKAudioBackend::update_latencies () { GET_PRIVATE_JACK_POINTER (_priv_jack); - jack_recompute_total_latencies (_priv_jack); + JACK_SERVER_CALL (jack_recompute_total_latencies (_priv_jack)); } ChanCount diff --git a/libs/backends/jack/jack_audiobackend.h b/libs/backends/jack/jack_audiobackend.h index f01c5e0e7e..2fb5da2276 100644 --- a/libs/backends/jack/jack_audiobackend.h +++ b/libs/backends/jack/jack_audiobackend.h @@ -324,7 +324,7 @@ class JACKAudioBackend : public AudioBackend { JACKSession* _session; - Glib::Threads::Mutex port_registration_mutex; + mutable Glib::Threads::Mutex server_call_mutex; protected: int _start (bool for_latency_measurement); diff --git a/libs/backends/jack/jack_portengine.cc b/libs/backends/jack/jack_portengine.cc index 7a5531b6ec..9205135e17 100644 --- a/libs/backends/jack/jack_portengine.cc +++ b/libs/backends/jack/jack_portengine.cc @@ -26,6 +26,7 @@ #include "jack_audiobackend.h" #include "jack_connection.h" +#include "ardour/debug.h" #include "ardour/port_manager.h" #include "pbd/i18n.h" @@ -37,6 +38,8 @@ using std::vector; #define GET_PRIVATE_JACK_POINTER(localvar) jack_client_t* localvar = _jack_connection->jack(); if (!(localvar)) { return; } #define GET_PRIVATE_JACK_POINTER_RET(localvar,r) jack_client_t* localvar = _jack_connection->jack(); if (!(localvar)) { return r; } +// #define JACK_SERVER_CALL(expr) { std::cerr << "JACK SERVER CALL: " << pthread_self() << '/' << pthread_name() << ' ' << #expr << std::endl; Glib::Threads::Mutex::Lock lm (server_call_mutex); expr; } +#define JACK_SERVER_CALL(expr) { Glib::Threads::Mutex::Lock lm (server_call_mutex); expr; } static uint32_t ardour_port_flags_to_jack_flags (PortFlags flags) @@ -99,9 +102,9 @@ JACKAudioBackend::when_connected_to_jack () return; } - jack_set_port_registration_callback (client, _registration_callback, this); - jack_set_port_connect_callback (client, _connect_callback, this); - jack_set_graph_order_callback (client, _graph_order_callback, this); + JACK_SERVER_CALL (jack_set_port_registration_callback (client, _registration_callback, this)); + JACK_SERVER_CALL (jack_set_port_connect_callback (client, _connect_callback, this)); + JACK_SERVER_CALL (jack_set_graph_order_callback (client, _graph_order_callback, this)); } int @@ -110,12 +113,12 @@ JACKAudioBackend::set_port_name (PortHandle port, const std::string& name) #if HAVE_JACK_PORT_RENAME jack_client_t* client = _jack_connection->jack(); if (client) { - return jack_port_rename (client, std::dynamic_pointer_cast(port)->jack_ptr, name.c_str()); + JACK_SERVER_CALL (return jack_port_rename (client, std::dynamic_pointer_cast(port)->jack_ptr, name.c_str())); } else { return -1; } #else - return jack_port_set_name (std::dynamic_pointer_cast(port)->jack_ptr, name.c_str()); + JACK_SERVER_CALL (return jack_port_set_name (std::dynamic_pointer_cast(port)->jack_ptr, name.c_str())); #endif } @@ -224,6 +227,8 @@ JACKAudioBackend::get_port_by_name (const std::string& name) const void JACKAudioBackend::_registration_callback (jack_port_id_t id, int reg, void* arg) { + DEBUG_TRACE (DEBUG::BackendCallbacks, string_compose ("%1/%2 jack port registration callback\n", pthread_self(), pthread_name())); + /* we don't use a virtual method for the registration callback, because JACK is the only backend that delivers the arguments shown above. So call our own JACK-centric registration callback, then the generic @@ -292,12 +297,14 @@ JACKAudioBackend::jack_registration_callback (jack_port_id_t id, int reg) int JACKAudioBackend::_graph_order_callback (void *arg) { + DEBUG_TRACE (DEBUG::BackendCallbacks, string_compose ("%1/%2 jack graph order callback\n", pthread_self(), pthread_name())); return static_cast (arg)->manager.graph_order_callback (); } void JACKAudioBackend::_connect_callback (jack_port_id_t id_a, jack_port_id_t id_b, int conn, void* arg) { + DEBUG_TRACE (DEBUG::BackendCallbacks, string_compose ("%1/%2 jack connect/disconnect callback\n", pthread_self(), pthread_name())); static_cast (arg)->connect_callback (id_a, id_b, conn); } @@ -327,7 +334,7 @@ JACKAudioBackend::connected (PortHandle p, bool process_callback_safe) ports = jack_port_get_connections (port); } else { GET_PRIVATE_JACK_POINTER_RET (_priv_jack, false); - ports = jack_port_get_all_connections (_priv_jack, port); + JACK_SERVER_CALL (ports = jack_port_get_all_connections (_priv_jack, port)); } if (ports) { @@ -350,7 +357,7 @@ JACKAudioBackend::connected_to (PortHandle p, const std::string& other, bool pro ports = jack_port_get_connections (port); } else { GET_PRIVATE_JACK_POINTER_RET (_priv_jack, false); - ports = jack_port_get_all_connections (_priv_jack, port); + JACK_SERVER_CALL (ports = jack_port_get_all_connections (_priv_jack, port)); } if (ports) { @@ -377,7 +384,7 @@ JACKAudioBackend::physically_connected (PortHandle p, bool process_callback_safe ports = jack_port_get_connections ((jack_port_t*)port); } else { GET_PRIVATE_JACK_POINTER_RET (_priv_jack, false); - ports = jack_port_get_all_connections (_priv_jack, (jack_port_t*)port); + JACK_SERVER_CALL (ports = jack_port_get_all_connections (_priv_jack, (jack_port_t*)port)); } if (ports) { @@ -408,7 +415,7 @@ JACKAudioBackend::externally_connected (PortHandle p, bool process_callback_safe ports = jack_port_get_connections ((jack_port_t*)port); } else { GET_PRIVATE_JACK_POINTER_RET (_priv_jack, false); - ports = jack_port_get_all_connections (_priv_jack, (jack_port_t*)port); + JACK_SERVER_CALL (ports = jack_port_get_all_connections (_priv_jack, (jack_port_t*)port)); } if (ports) { @@ -439,7 +446,7 @@ JACKAudioBackend::get_connections (PortHandle p, vector& s, bool process ports = jack_port_get_connections (port); } else { GET_PRIVATE_JACK_POINTER_RET (_priv_jack, 0); - ports = jack_port_get_all_connections (_priv_jack, port); + JACK_SERVER_CALL (ports = jack_port_get_all_connections (_priv_jack, port)); } if (ports) { @@ -480,9 +487,7 @@ JACKAudioBackend::get_ports (const string& port_name_pattern, DataType type, Por GET_PRIVATE_JACK_POINTER_RET (_priv_jack,0); - const char** ports = jack_get_ports (_priv_jack, port_name_pattern.c_str(), - ardour_data_type_to_jack_port_type (type), - ardour_port_flags_to_jack_flags (flags)); + const char** ports = jack_get_ports (_priv_jack, port_name_pattern.c_str(), ardour_data_type_to_jack_port_type (type), ardour_port_flags_to_jack_flags (flags)); if (ports == 0) { return 0; @@ -513,21 +518,19 @@ void JACKAudioBackend::get_physical (DataType type, unsigned long flags, vector& phy) const { GET_PRIVATE_JACK_POINTER (_priv_jack); - const char ** ports; + const char ** ports = jack_get_ports (_priv_jack, NULL, ardour_data_type_to_jack_port_type (type), JackPortIsPhysical | flags); - if ((ports = jack_get_ports (_priv_jack, NULL, ardour_data_type_to_jack_port_type (type), JackPortIsPhysical | flags)) == 0) { + if (!ports) { return; } - if (ports) { - for (uint32_t i = 0; ports[i]; ++i) { - if (strstr (ports[i], "Midi-Through")) { - continue; - } - phy.push_back (ports[i]); + for (uint32_t i = 0; ports[i]; ++i) { + if (strstr (ports[i], "Midi-Through")) { + continue; } - jack_free (ports); + phy.push_back (ports[i]); } + jack_free (ports); } /** Get physical ports for which JackPortIsOutput is set; ie those that correspond to @@ -553,9 +556,9 @@ bool JACKAudioBackend::can_monitor_input () const { GET_PRIVATE_JACK_POINTER_RET (_priv_jack,false); - const char ** ports; + const char ** ports = jack_get_ports (_priv_jack, NULL, JACK_DEFAULT_AUDIO_TYPE, JackPortCanMonitor); - if ((ports = jack_get_ports (_priv_jack, NULL, JACK_DEFAULT_AUDIO_TYPE, JackPortCanMonitor)) == 0) { + if (ports) { return false; } @@ -567,12 +570,12 @@ JACKAudioBackend::can_monitor_input () const int JACKAudioBackend::request_input_monitoring (PortHandle port, bool yn) { - return jack_port_request_monitor (std::dynamic_pointer_cast(port)->jack_ptr, yn); + JACK_SERVER_CALL (return jack_port_request_monitor (std::dynamic_pointer_cast(port)->jack_ptr, yn)); } int JACKAudioBackend::ensure_input_monitoring (PortHandle port, bool yn) { - return jack_port_ensure_monitor (std::dynamic_pointer_cast(port)->jack_ptr, yn); + JACK_SERVER_CALL (return jack_port_ensure_monitor (std::dynamic_pointer_cast(port)->jack_ptr, yn)); } bool JACKAudioBackend::monitoring_input (PortHandle port) @@ -584,14 +587,9 @@ PortEngine::PortPtr JACKAudioBackend::register_port (const std::string& shortname, ARDOUR::DataType type, ARDOUR::PortFlags flags) { jack_port_t* jack_port; - { - Glib::Threads::Mutex::Lock lm (port_registration_mutex); - GET_PRIVATE_JACK_POINTER_RET (_priv_jack, PortEngine::PortPtr()); - jack_port = jack_port_register (_priv_jack, shortname.c_str(), - ardour_data_type_to_jack_port_type (type), - ardour_port_flags_to_jack_flags (flags), - 0); - } + + GET_PRIVATE_JACK_POINTER_RET (_priv_jack, PortEngine::PortPtr()); + JACK_SERVER_CALL (jack_port = jack_port_register (_priv_jack, shortname.c_str(), ardour_data_type_to_jack_port_type (type), ardour_port_flags_to_jack_flags (flags), 0)); if (!jack_port) { return PortEngine::PortPtr(); @@ -635,7 +633,10 @@ int JACKAudioBackend::connect (PortHandle port, const std::string& other) { GET_PRIVATE_JACK_POINTER_RET (_priv_jack, -1); - int r = jack_connect (_priv_jack, jack_port_name (std::dynamic_pointer_cast(port)->jack_ptr), other.c_str()); + int r; + + JACK_SERVER_CALL (r = jack_connect (_priv_jack, jack_port_name (std::dynamic_pointer_cast(port)->jack_ptr), other.c_str())); + if (r == 0 || r == EEXIST) { return 0; } @@ -645,11 +646,14 @@ int JACKAudioBackend::connect (const std::string& src, const std::string& dst) { GET_PRIVATE_JACK_POINTER_RET (_priv_jack, -1); + int r; + + JACK_SERVER_CALL (r = jack_connect (_priv_jack, src.c_str(), dst.c_str())); - int r = jack_connect (_priv_jack, src.c_str(), dst.c_str()); if (r == 0 || r == EEXIST) { return 0; } + return r; } @@ -657,21 +661,21 @@ int JACKAudioBackend::disconnect (PortHandle port, const std::string& other) { GET_PRIVATE_JACK_POINTER_RET (_priv_jack, -1); - return jack_disconnect (_priv_jack, jack_port_name (std::dynamic_pointer_cast(port)->jack_ptr), other.c_str()); + JACK_SERVER_CALL (return jack_disconnect (_priv_jack, jack_port_name (std::dynamic_pointer_cast(port)->jack_ptr), other.c_str())); } int JACKAudioBackend::disconnect (const std::string& src, const std::string& dst) { GET_PRIVATE_JACK_POINTER_RET (_priv_jack, -1); - return jack_disconnect (_priv_jack, src.c_str(), dst.c_str()); + JACK_SERVER_CALL (return jack_disconnect (_priv_jack, src.c_str(), dst.c_str())); } int JACKAudioBackend::disconnect_all (PortHandle port) { GET_PRIVATE_JACK_POINTER_RET (_priv_jack, -1); - return jack_port_disconnect (_priv_jack, std::dynamic_pointer_cast(port)->jack_ptr); + JACK_SERVER_CALL (return jack_port_disconnect (_priv_jack, std::dynamic_pointer_cast(port)->jack_ptr)); } int