From b13d7d554d8047a770d15ea1bd57c828e405792e Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Sun, 19 Apr 2020 16:14:22 +0200 Subject: [PATCH] Prevent rare race conditions with get/setParameter I've never seen it happen, but in theory they could be called simultaneously. --- src/common/events.h | 6 +++--- src/plugin/host-bridge.cpp | 29 +++++++++++++++++++++-------- src/plugin/host-bridge.h | 13 ++++++++----- 3 files changed, 32 insertions(+), 16 deletions(-) diff --git a/src/common/events.h b/src/common/events.h index 2293350d..5155abfb 100644 --- a/src/common/events.h +++ b/src/common/events.h @@ -102,7 +102,7 @@ class DefaultDataConverter { * * @param socket The socket to write over, should be the same socket the other * endpoint is using to call `passthrough_event()`. - * @param write_semaphore A mutex to ensure that only one thread can write to + * @param write_mutex A mutex to ensure that only one thread can write to * the socket at once. Needed because VST hosts and plugins can and sometimes * will call the `dispatch()` or `audioMaster()` functions from multiple * threads at once. @@ -120,7 +120,7 @@ class DefaultDataConverter { */ template intptr_t send_event(boost::asio::local::stream_protocol::socket& socket, - std::mutex& write_semaphore, + std::mutex& write_mutex, D& data_converter, std::optional> logging, int opcode, @@ -153,7 +153,7 @@ intptr_t send_event(boost::asio::local::stream_protocol::socket& socket, // multiple threads. EventResult response; { - std::lock_guard lock(write_semaphore); + std::lock_guard lock(write_mutex); write_object(socket, event); response = read_object(socket); } diff --git a/src/plugin/host-bridge.cpp b/src/plugin/host-bridge.cpp index 16075018..79f858b6 100644 --- a/src/plugin/host-bridge.cpp +++ b/src/plugin/host-bridge.cpp @@ -290,7 +290,7 @@ intptr_t HostBridge::dispatch(AEffect* /*plugin*/, intptr_t return_value = 1; try { return_value = - send_event(host_vst_dispatch, dispatch_semaphore, converter, + send_event(host_vst_dispatch, dispatch_mutex, converter, std::pair(logger, true), opcode, index, value, data, option); } catch (const boost::system::system_error& a) { @@ -330,14 +330,14 @@ intptr_t HostBridge::dispatch(AEffect* /*plugin*/, // stop receiving midi data when they have an open dropdowns or // message box. return send_event(host_vst_dispatch_midi_events, - dispatch_midi_events_semaphore, converter, + dispatch_midi_events_mutex, converter, std::pair(logger, true), opcode, index, value, data, option); break; } // TODO: Maybe reuse buffers here when dealing with chunk data - return send_event(host_vst_dispatch, dispatch_semaphore, converter, + return send_event(host_vst_dispatch, dispatch_mutex, converter, std::pair(logger, true), opcode, index, value, data, option); } @@ -374,9 +374,16 @@ float HostBridge::get_parameter(AEffect* /*plugin*/, int index) { logger.log_get_parameter(index); const Parameter request{index, std::nullopt}; - write_object(host_vst_parameters, request); + ParameterResult response; + + // Prevent race conditions from `getParameter()` and `setParameter()` being + // called at the same time since they share the same socket + { + std::lock_guard lock(parameters_mutex); + write_object(host_vst_parameters, request); + response = read_object(host_vst_parameters); + } - const auto response = read_object(host_vst_parameters); logger.log_get_parameter_response(response.value.value()); return response.value.value(); @@ -386,12 +393,18 @@ void HostBridge::set_parameter(AEffect* /*plugin*/, int index, float value) { logger.log_set_parameter(index, value); const Parameter request{index, value}; - write_object(host_vst_parameters, request); + ParameterResult response; + + { + std::lock_guard lock(parameters_mutex); + write_object(host_vst_parameters, request); + + response = read_object(host_vst_parameters); + } - // This should not contain any values and just serve as an acknowledgement - const auto response = read_object(host_vst_parameters); logger.log_set_parameter_response(); + // This should not contain any values and just serve as an acknowledgement assert(!response.value.has_value()); } diff --git a/src/plugin/host-bridge.h b/src/plugin/host-bridge.h index bdcbfbae..cfca3ca0 100644 --- a/src/plugin/host-bridge.h +++ b/src/plugin/host-bridge.h @@ -150,9 +150,6 @@ class HostBridge { /** * Used for both `getParameter` and `setParameter` since they mostly * overlap. - * - * TODO: Verify that these 100% won't be called simultanously since that - * would cause a race condition. */ boost::asio::local::stream_protocol::socket host_vst_parameters; boost::asio::local::stream_protocol::socket host_vst_process_replacing; @@ -174,8 +171,14 @@ class HostBridge { * being called by two threads at once. See `send_event()` for more * information. */ - std::mutex dispatch_semaphore; - std::mutex dispatch_midi_events_semaphore; + std::mutex dispatch_mutex; + std::mutex dispatch_midi_events_mutex; + /** + * A similar semaphore as the `dispatch_*` semaphores in the rare case that + * `getParameter()` and `setParameter()` are being called at the same time + * since they use the same socket. + */ + std::mutex parameters_mutex; /** * The callback function passed by the host to the VST plugin instance.