From e07467697ae883816f0bf48676a7e1456615ddde Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Thu, 10 Dec 2020 12:03:51 +0100 Subject: [PATCH 1/8] Only wait for sockets during initialization #69 --- CHANGELOG.md | 8 ++++++ src/common/communication.h | 55 ++++++++++++++++++++++++++++++++------ 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 78db3340..3cd2505f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,14 @@ Versioning](https://semver.org/spec/v2.0.0.html). - Symbols in all `libyabridge.so` and all Winelib `.so` files are now hidden by default. +### Fixed + +- Fixed an issue where in certain situations Wine processes were left running + after the host got forcefully terminated before it got a chance to tell the + plugin to shut down. This could happen when using Kontakt in Bitwig, since + BItwig sets a limit on the amount of time a plugin may take to shut down when + closing Bitwig. + ## [2.1.0] - 2020-11-20 ### Added diff --git a/src/common/communication.h b/src/common/communication.h index b9332ba3..de8c2e97 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -413,8 +413,12 @@ class EventHandler { */ EventHandler(boost::asio::io_context& io_context, boost::asio::local::stream_protocol::endpoint endpoint, - bool listen) - : io_context(io_context), endpoint(endpoint), socket(io_context) { + bool listen, + std::atomic_bool& are_sockets_connected) + : io_context(io_context), + endpoint(endpoint), + socket(io_context), + are_sockets_connected(are_sockets_connected) { if (listen) { boost::filesystem::create_directories( boost::filesystem::path(endpoint.path()).parent_path()); @@ -527,7 +531,7 @@ class EventHandler { write_object(secondary_socket, event); response = read_object(secondary_socket); - } catch (const boost::system::system_error&) { + } catch (const boost::system::system_error& e) { // So, what do we do when noone is listening on the endpoint // yet? This can happen with plugin groups when the Wine // host process does an `audioMaster()` call before the @@ -536,10 +540,20 @@ class EventHandler { // anyone can think of a better way to structure all of this // while still mainting a long living primary socket please // let me know. - std::lock_guard lock(write_mutex); + // Note that this should **only** be done before the call to + // `connect()`. If we get here at any other point then it + // means that the plugin side is no longer listening on the + // sockets, and we should thus just exit. + if (!are_sockets_connected) { + std::lock_guard lock(write_mutex); - write_object(socket, event); - response = read_object(socket); + write_object(socket, event); + response = read_object(socket); + } else { + // Rethrow the exception if the sockets we're not + // handling the specific case described above + throw e; + } } } } @@ -748,6 +762,11 @@ class EventHandler { * events will be sent over a new socket instead. */ std::mutex write_mutex; + + /** + * @Sockets::are_sockets_connected + */ + std::atomic_bool& are_sockets_connected; }; /** @@ -787,10 +806,12 @@ class Sockets { : base_dir(endpoint_base_dir), host_vst_dispatch(io_context, (base_dir / "host_vst_dispatch.sock").string(), - listen), + listen, + are_sockets_connected), vst_host_callback(io_context, (base_dir / "vst_host_callback.sock").string(), - listen), + listen, + are_sockets_connected), host_vst_parameters(io_context, (base_dir / "host_vst_parameters.sock").string(), listen), @@ -837,8 +858,17 @@ class Sockets { host_vst_parameters.connect(); host_vst_process_replacing.connect(); host_vst_control.connect(); + + are_sockets_connected = true; } + /** + * Whether or `connect()` has been called already. When a plugin host a host + * callback during its initialization, before the sockets may have been set + * up, then we'll want to wait for the sockets to be set up. + */ + inline bool connected() { return are_sockets_connected; } + /** * The base directory for our socket endpoints. All `*_endpoint` variables * below are files within this directory. @@ -877,6 +907,15 @@ class Sockets { * the configuration (from `config`) back to the Wine host. */ SocketHandler host_vst_control; + + private: + /** + * Indicates whether we have already called `connect()` or not. When a + * Windows VST2 plugin performs a host callback in its constructor, before + * the native plugin has had time to connect to the sockets, we want it to + * always wait for the sockets to come online. + */ + std::atomic_bool are_sockets_connected = false; }; /** From 5f7c105b6d8998bc4aac460e47241bbdfdb1d066 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Thu, 10 Dec 2020 15:03:26 +0100 Subject: [PATCH 2/8] Use boost::asio::transfer_exactly Instead of doing assertions. --- src/common/communication.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/common/communication.h b/src/common/communication.h index de8c2e97..3addf61d 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -106,7 +106,8 @@ template inline T read_object(Socket& socket, std::vector& buffer) { // See the note above on the use of `uint64_t` instead of `size_t` std::array message_length; - boost::asio::read(socket, boost::asio::buffer(message_length)); + boost::asio::read(socket, boost::asio::buffer(message_length), + boost::asio::transfer_exactly(sizeof(message_length))); // Make sure the buffer is large enough const size_t size = message_length[0]; @@ -116,8 +117,8 @@ inline T read_object(Socket& socket, std::vector& buffer) { // merging for us, since local domain sockets have packet limits somewhere // in the hundreds of kilobytes const auto actual_size = - boost::asio::read(socket, boost::asio::buffer(buffer)); - assert(size == actual_size); + boost::asio::read(socket, boost::asio::buffer(buffer), + boost::asio::transfer_exactly(size)); T object; auto [_, success] = From 5c98b74bc2161d2b3eab062dda063f2e0dea89bb Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Thu, 10 Dec 2020 15:07:16 +0100 Subject: [PATCH 3/8] Remove now unused variable --- src/common/communication.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/common/communication.h b/src/common/communication.h index 3addf61d..0bab1d5d 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -116,9 +116,8 @@ inline T read_object(Socket& socket, std::vector& buffer) { // `boost::asio::read/write` will handle all the packet splitting and // merging for us, since local domain sockets have packet limits somewhere // in the hundreds of kilobytes - const auto actual_size = - boost::asio::read(socket, boost::asio::buffer(buffer), - boost::asio::transfer_exactly(size)); + boost::asio::read(socket, boost::asio::buffer(buffer), + boost::asio::transfer_exactly(size)); T object; auto [_, success] = From e9af76d21e13c5b65b15cd886fc164d30bb8fc5a Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Thu, 10 Dec 2020 15:44:08 +0100 Subject: [PATCH 4/8] Get rid of all traces of pthreads in Wine Since this can only cause issues. --- meson.build | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/meson.build b/meson.build index 9e1eddcf..a94c8ef0 100644 --- a/meson.build +++ b/meson.build @@ -65,8 +65,6 @@ bitsery_dep = subproject('bitsery', version : '5.2.0').get_variable('bitsery_dep function2_dep = subproject('function2', version : '4.1.0').get_variable('function2_dep') threads_dep = dependency('threads') tomlplusplus_dep = subproject('tomlplusplus', version : '2.1.0').get_variable('tomlplusplus_dep') -# The built in threads dependency does not know how to handle winegcc -wine_threads_dep = declare_dependency(link_args : '-lpthread') xcb_dep = dependency('xcb') include_dir = include_directories('src/include') @@ -122,6 +120,10 @@ group_host_sources = host_sources + [ 'src/wine-host/group-host.cpp', ] +# Get rid of all traces of pthreads in Boost.Asio since this will not play +# nicely with Wine's own thread system +compiler_options += ['-DBOOST_ASIO_DISABLE_THREADS', '-DBOOST_ASIO_DISABLE_STD_THREAD'] + executable( individual_host_name_64bit, individual_host_sources, @@ -133,7 +135,6 @@ executable( bitsery_dep, function2_dep, tomlplusplus_dep, - wine_threads_dep, xcb_dep ], cpp_args : compiler_options + ['-m64'], @@ -151,7 +152,6 @@ executable( bitsery_dep, function2_dep, tomlplusplus_dep, - wine_threads_dep, xcb_dep ], cpp_args : compiler_options + ['-m64'], @@ -197,7 +197,6 @@ if with_bitbridge bitsery_dep, function2_dep, tomlplusplus_dep, - wine_threads_dep, xcb_dep ], cpp_args : compiler_options + ['-m32'], @@ -215,7 +214,6 @@ if with_bitbridge bitsery_dep, function2_dep, tomlplusplus_dep, - wine_threads_dep, xcb_dep ], cpp_args : compiler_options + ['-m32'], From e3a52bca147175d7eda3f3aeb9591e1b5039dbf4 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Thu, 10 Dec 2020 17:37:43 +0100 Subject: [PATCH 5/8] Revert "Get rid of all traces of pthreads in Wine" This reverts commit e9af76d21e13c5b65b15cd886fc164d30bb8fc5a. Without this the event loop will just not run, not sure why. This needs some more investigation. --- meson.build | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/meson.build b/meson.build index a94c8ef0..9e1eddcf 100644 --- a/meson.build +++ b/meson.build @@ -65,6 +65,8 @@ bitsery_dep = subproject('bitsery', version : '5.2.0').get_variable('bitsery_dep function2_dep = subproject('function2', version : '4.1.0').get_variable('function2_dep') threads_dep = dependency('threads') tomlplusplus_dep = subproject('tomlplusplus', version : '2.1.0').get_variable('tomlplusplus_dep') +# The built in threads dependency does not know how to handle winegcc +wine_threads_dep = declare_dependency(link_args : '-lpthread') xcb_dep = dependency('xcb') include_dir = include_directories('src/include') @@ -120,10 +122,6 @@ group_host_sources = host_sources + [ 'src/wine-host/group-host.cpp', ] -# Get rid of all traces of pthreads in Boost.Asio since this will not play -# nicely with Wine's own thread system -compiler_options += ['-DBOOST_ASIO_DISABLE_THREADS', '-DBOOST_ASIO_DISABLE_STD_THREAD'] - executable( individual_host_name_64bit, individual_host_sources, @@ -135,6 +133,7 @@ executable( bitsery_dep, function2_dep, tomlplusplus_dep, + wine_threads_dep, xcb_dep ], cpp_args : compiler_options + ['-m64'], @@ -152,6 +151,7 @@ executable( bitsery_dep, function2_dep, tomlplusplus_dep, + wine_threads_dep, xcb_dep ], cpp_args : compiler_options + ['-m64'], @@ -197,6 +197,7 @@ if with_bitbridge bitsery_dep, function2_dep, tomlplusplus_dep, + wine_threads_dep, xcb_dep ], cpp_args : compiler_options + ['-m32'], @@ -214,6 +215,7 @@ if with_bitbridge bitsery_dep, function2_dep, tomlplusplus_dep, + wine_threads_dep, xcb_dep ], cpp_args : compiler_options + ['-m32'], From c05040d98b6dad3275bf522ce28c4b192dcf4e2a Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Thu, 10 Dec 2020 18:01:11 +0100 Subject: [PATCH 6/8] Link with -mconsole instead of -mwindows Not sure why it even worked with -mwindows, but this is the correct linking option for the `main()` entry point. --- CHANGELOG.md | 6 +++--- cross-wine.conf | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cd2505f..e6347b4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,9 +32,9 @@ Versioning](https://semver.org/spec/v2.0.0.html). - Fixed an issue where in certain situations Wine processes were left running after the host got forcefully terminated before it got a chance to tell the - plugin to shut down. This could happen when using Kontakt in Bitwig, since - BItwig sets a limit on the amount of time a plugin may take to shut down when - closing Bitwig. + plugin to shut down. This could happen when using Kontakt in Bitwig, as Bitwig + sets a limit on the amount of time a plugin may take to shut down when closing + Bitwig. ## [2.1.0] - 2020-11-20 diff --git a/cross-wine.conf b/cross-wine.conf index 1033d8d7..0d7ff22b 100644 --- a/cross-wine.conf +++ b/cross-wine.conf @@ -17,9 +17,9 @@ needs_exe_wrapper = true # applications are always in sync. This might not be needed anymore once Meson # cross compiling to multiple targets at once. # https://github.com/mesonbuild/meson/issues/5125 -cpp_link_args = ['-mwindows'] +cpp_link_args = ['-mconsole'] # For instance, this should be in the 64-bit only cross-file # c_args = ['-m64'] # cpp_args = ['-m64'] -# cpp_link_args = ['-m64', '-mwindows'] +# cpp_link_args = ['-m64', '-mconsole'] From ac0d83e5553c4131a8b9faefbcbd9a0e82c77f36 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Thu, 10 Dec 2020 21:40:52 +0100 Subject: [PATCH 7/8] Fix concurrency issue in plugin group shutdown --- CHANGELOG.md | 2 ++ src/wine-host/bridges/group.cpp | 3 ++- src/wine-host/bridges/group.h | 5 +++++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e6347b4f..08de7811 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,8 @@ Versioning](https://semver.org/spec/v2.0.0.html). plugin to shut down. This could happen when using Kontakt in Bitwig, as Bitwig sets a limit on the amount of time a plugin may take to shut down when closing Bitwig. +- Fixed a potential crash or freeze when removing a lot of plugins from a plugin + group at exactly the same time. ## [2.1.0] - 2020-11-20 diff --git a/src/wine-host/bridges/group.cpp b/src/wine-host/bridges/group.cpp index d81788ca..164cabf8 100644 --- a/src/wine-host/bridges/group.cpp +++ b/src/wine-host/bridges/group.cpp @@ -135,8 +135,9 @@ void GroupBridge::handle_plugin_dispatch(size_t plugin_id) { // Defer actually shutting down the process to allow for fast plugin // scanning by allowing plugins to reuse the same group host process + std::lock_guard lock(shutdown_timer_mutex); shutdown_timer.expires_after(2s); - shutdown_timer.async_wait([&](const boost::system::error_code& error) { + shutdown_timer.async_wait([this](const boost::system::error_code& error) { // A previous timer gets canceled automatically when another plugin // exits if (error.failed()) { diff --git a/src/wine-host/bridges/group.h b/src/wine-host/bridges/group.h index 0313d249..6dfccee7 100644 --- a/src/wine-host/bridges/group.h +++ b/src/wine-host/bridges/group.h @@ -276,4 +276,9 @@ class GroupBridge { * @see handle_plugin_dispatch */ boost::asio::steady_timer shutdown_timer; + /** + * A mutex to prevent two threads from simultaneously modifying the shutdown + * timer when multiple plugins exit at the same time. + */ + std::mutex shutdown_timer_mutex; }; From 2615da51da07ddf45c16f0d3602e3b7b59b0016b Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Fri, 11 Dec 2020 00:32:03 +0100 Subject: [PATCH 8/8] Fix the socket waiting fix #69 e07467697ae883816f0bf48676a7e1456615ddde changed the waiting behaviour, but this meant that there was a very slight window where all secondary requests would fail when both sides have called connect(), but the other side has not already called `receive_{events,multi,messages}` to start listening on the socket. --- src/common/communication.h | 47 +++++++++++++------------------------- 1 file changed, 16 insertions(+), 31 deletions(-) diff --git a/src/common/communication.h b/src/common/communication.h index 0bab1d5d..968ac1e6 100644 --- a/src/common/communication.h +++ b/src/common/communication.h @@ -413,12 +413,8 @@ class EventHandler { */ EventHandler(boost::asio::io_context& io_context, boost::asio::local::stream_protocol::endpoint endpoint, - bool listen, - std::atomic_bool& are_sockets_connected) - : io_context(io_context), - endpoint(endpoint), - socket(io_context), - are_sockets_connected(are_sockets_connected) { + bool listen) + : io_context(io_context), endpoint(endpoint), socket(io_context) { if (listen) { boost::filesystem::create_directories( boost::filesystem::path(endpoint.path()).parent_path()); @@ -544,7 +540,7 @@ class EventHandler { // `connect()`. If we get here at any other point then it // means that the plugin side is no longer listening on the // sockets, and we should thus just exit. - if (!are_sockets_connected) { + if (!sent_first_event) { std::lock_guard lock(write_mutex); write_object(socket, event); @@ -558,6 +554,10 @@ class EventHandler { } } + // This was used to always block when sending the first message, because + // the other side may not be listening for additional connections yet + sent_first_event = true; + if (logging) { auto [logger, is_dispatch] = *logging; logger.log_event_response(is_dispatch, opcode, @@ -763,10 +763,15 @@ class EventHandler { */ std::mutex write_mutex; + private: /** - * @Sockets::are_sockets_connected + * Indicates whether or not the remove has processed an event we sent from + * this side. When a Windows VST2 plugin performs a host callback in its + * constructor, before the native plugin has had time to connect to the + * sockets, we want it to always wait for the sockets to come online, but + * this fallback behaviour should only happen during initialization. */ - std::atomic_bool& are_sockets_connected; + std::atomic_bool sent_first_event = false; }; /** @@ -806,12 +811,10 @@ class Sockets { : base_dir(endpoint_base_dir), host_vst_dispatch(io_context, (base_dir / "host_vst_dispatch.sock").string(), - listen, - are_sockets_connected), + listen), vst_host_callback(io_context, (base_dir / "vst_host_callback.sock").string(), - listen, - are_sockets_connected), + listen), host_vst_parameters(io_context, (base_dir / "host_vst_parameters.sock").string(), listen), @@ -858,17 +861,8 @@ class Sockets { host_vst_parameters.connect(); host_vst_process_replacing.connect(); host_vst_control.connect(); - - are_sockets_connected = true; } - /** - * Whether or `connect()` has been called already. When a plugin host a host - * callback during its initialization, before the sockets may have been set - * up, then we'll want to wait for the sockets to be set up. - */ - inline bool connected() { return are_sockets_connected; } - /** * The base directory for our socket endpoints. All `*_endpoint` variables * below are files within this directory. @@ -907,15 +901,6 @@ class Sockets { * the configuration (from `config`) back to the Wine host. */ SocketHandler host_vst_control; - - private: - /** - * Indicates whether we have already called `connect()` or not. When a - * Windows VST2 plugin performs a host callback in its constructor, before - * the native plugin has had time to connect to the sockets, we want it to - * always wait for the sockets to come online. - */ - std::atomic_bool are_sockets_connected = false; }; /**