Prevent data races in host callbacks

README.md

@@ -12,8 +12,6 @@ There are a few things that should be done before releasing this, including:
   - All effect plugins report that they have a sidechain input and multiple
     outputs. This might be because of the initial value of the `AEffect` struct
     we pass to the host callbacks during initialization? Not sure.
-  - Add a similar seaphore to the plugin's `audioMaster` callback handler to
-    prevent requests from multiple threads being sent at once.
 - Add missing details if any to the architecture section.
 - Document what this has been tested on and what does or does not work.
 - Document wine32 support.

src/common/events.h

@@ -16,6 +16,8 @@
 
 #pragma once
 
+#include <mutex>
+
 #include "communication.h"
 #include "logging.h"
 
@@ -97,6 +99,12 @@ class DefaultDataConverter {
  * since they follow the same format. See one of those functions for details on
  * the parameters and return value of this function.
  *
+ * @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
+ *   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.
  * @param data_converter Some struct that knows how to read data from and write
  *   data back to the `data` void pointer. For host callbacks this parameter
  *   contains either a string or a null pointer while `dispatch()` calls might
@@ -111,6 +119,7 @@ class DefaultDataConverter {
  */
 template <typename D>
 intptr_t send_event(boost::asio::local::stream_protocol::socket& socket,
+                    std::mutex& write_semaphore,
                     D& data_converter,
                     int opcode,
                     int index,
@@ -135,7 +144,14 @@ intptr_t send_event(boost::asio::local::stream_protocol::socket& socket,
     }
 
     const Event event{opcode, index, value, option, payload.value()};
+
+    // Prevent two threads from writing over the socket at the same time. This
+    // should not be needed, but for instance Bitwig's plugin bridge will
+    // sometimes repeatedly send events from an off thread that may overlap with
+    // other `dispatch()` calls.
+    write_semaphore.lock();
     write_object(socket, event);
+    write_semaphore.unlock();
 
     const auto response = read_object<EventResult>(socket);
 

src/plugin/host-bridge.cpp

@@ -262,8 +262,8 @@ intptr_t HostBridge::dispatch(AEffect* /*plugin*/,
 
             // Allow the plugin to handle its own shutdown
             const auto return_value = send_event(
-                host_vst_dispatch, converter, opcode, index, value, data,
-                option, std::pair<Logger&, bool>(logger, true));
+                host_vst_dispatch, dispatch_semaphore, converter, opcode, index,
+                value, data, option, std::pair<Logger&, bool>(logger, true));
 
             // Boost.Process will send SIGKILL to the Wien host for us when this
             // class gets destroyed. Because the process is running a few
@@ -293,20 +293,9 @@ intptr_t HostBridge::dispatch(AEffect* /*plugin*/,
     }
 
     // TODO: Maybe reuse buffers here when dealing with chunk data
-
-    // Finish message Bitwig's plugin host sometimes calls the dispatch function
-    // for opcode 52 from an off thread. This shouldn't be happening, but it
-    // does. To prevent race conditions from multiple writes over the same
-    // socket at once we'll make sure that only one thread can send sockets at
-    // once. These locks are actually only needed around the `write_object()`
-    // part of `send_event()`.
-    dispatch_semaphore.lock();
-    const auto return_value =
-        send_event(host_vst_dispatch, converter, opcode, index, value, data,
-                   option, std::pair<Logger&, bool>(logger, true));
-    dispatch_semaphore.unlock();
-
-    return return_value;
+    return send_event(host_vst_dispatch, dispatch_semaphore, converter, opcode,
+                      index, value, data, option,
+                      std::pair<Logger&, bool>(logger, true));
 }
 
 void HostBridge::process_replacing(AEffect* /*plugin*/,

src/plugin/host-bridge.h

@@ -158,12 +158,9 @@ class HostBridge {
     std::thread host_callback_handler;
 
     /**
-     * A binary semaphore for preventing the dispatch function from being called
-     * by two threads at once. This rarely happens and shouldn't event really be
-     * happening, but in Bitwig's plugin bridge sometimes calls the dispatch
-     * function with opcode 52 while it's still waiting for another dispatch
-     * call to return, which would cause two threads to write over the same
-     * socket at the same time.
+     * A binary semaphore to prevent race conditions from the dispatch function
+     * being called by two threads at once. See `send_event()` for more
+     * information.
      */
     std::mutex dispatch_semaphore;
 

src/wine-host/plugin-bridge.cpp

@@ -268,8 +268,8 @@ intptr_t PluginBridge::host_callback(AEffect* /*plugin*/,
                                      void* data,
                                      float option) {
     HostCallbackDataConverter converter(plugin, time_info);
-    return send_event(vst_host_callback, converter, opcode, index, value, data,
-                      option, std::nullopt);
+    return send_event(vst_host_callback, host_callback_semaphore, converter,
+                      opcode, index, value, data, option, std::nullopt);
 }
 
 intptr_t VST_CALL_CONV host_callback_proxy(AEffect* effect,

src/wine-host/plugin-bridge.h

@@ -27,6 +27,7 @@
 #include <windows.h>
 
 #include <boost/asio/local/stream_protocol.hpp>
+#include <mutex>
 #include <thread>
 
 #include "../common/logging.h"
@@ -115,6 +116,13 @@ class PluginBridge {
      */
     std::thread process_replacing_handler;
 
+    /**
+     * A binary semaphore to prevent race conditions from the host callback
+     * function being called by two threads at once. See `send_event()` for more
+     * information.
+     */
+    std::mutex host_callback_semaphore;
+
     /**
      * A scratch buffer for sending and receiving data during `process` and
      * `processReplacing` calls.