Revert event loop APIs to use Weak instead of Arc

Previously the plugin wouldn't get dropped because the event loops kept
them alive. These APIs used Weak instead of Arc in the past precisely to
prevent this issue. Not sure why I changed that.

src/event_loop.rs

@@ -1,6 +1,6 @@
 //! An internal event loop for spooling tasks to the/a GUI thread.
 
-use std::sync::Arc;
+use std::sync::Weak;
 
 mod background_thread;
 
@@ -46,7 +46,7 @@ where
 {
     /// Create and start a new event loop. The thread this is called on will be designated as the
     /// main thread, so this should be called when constructing the wrapper.
-    fn new_and_spawn(executor: Arc<E>) -> Self;
+    fn new_and_spawn(executor: Weak<E>) -> Self;
 
     /// Either post the function to the task queue so it can be delegated to the main thread, or
     /// execute the task directly if this is the main thread. This function needs to be callable at

src/event_loop/background_thread.rs

@@ -19,7 +19,7 @@ pub(crate) struct BackgroundThread<T, E> {
     /// The object that actually executes the task `T`. We'll send a weak reference to this to the
     /// worker thread whenever a task needs to be executed. This allows multiple plugin instances to
     /// share the same worker thread.
-    executor: Arc<E>,
+    executor: Weak<E>,
     /// A thread that act as our worker thread. When [`schedule()`][Self::schedule()] is called,
     /// this thread will be woken up to execute the task on the executor. When the last worker
     /// thread handle gets dropped the thread is shut down.
@@ -52,7 +52,7 @@ where
     T: Send + 'static,
     E: MainThreadExecutor<T> + 'static,
 {
-    pub fn get_or_create(executor: Arc<E>) -> Self {
+    pub fn get_or_create(executor: Weak<E>) -> Self {
         Self {
             executor,
             // The same worker thread can be shared by multiple instances. Lifecycle management
@@ -67,7 +67,7 @@ where
         permit_alloc(|| {
             self.worker_thread
                 .tasks_sender
-                .try_send(Message::Task((task, Arc::downgrade(&self.executor))))
+                .try_send(Message::Task((task, self.executor.clone())))
                 .is_ok()
         })
     }

src/event_loop/linux.rs

@@ -2,7 +2,7 @@
 //! of a main thread does not exist there. Because of that, this mostly just serves as a way to
 //! delegate expensive processing to another thread.
 
-use std::sync::Arc;
+use std::sync::Weak;
 use std::thread::{self, ThreadId};
 
 use super::{BackgroundThread, EventLoop, MainThreadExecutor};
@@ -13,7 +13,7 @@ pub(crate) struct LinuxEventLoop<T, E> {
     /// The thing that ends up executing these tasks. The tasks are usually executed from the worker
     /// thread, but if the current thread is the main thread then the task cna also be executed
     /// directly.
-    executor: Arc<E>,
+    executor: Weak<E>,
 
     /// The actual background thread. The implementation is shared with the background thread used
     /// in other backends.
@@ -29,7 +29,7 @@ where
     T: Send + 'static,
     E: MainThreadExecutor<T> + 'static,
 {
-    fn new_and_spawn(executor: Arc<E>) -> Self {
+    fn new_and_spawn(executor: Weak<E>) -> Self {
         Self {
             executor: executor.clone(),
             background_thread: BackgroundThread::get_or_create(executor),
@@ -39,7 +39,13 @@ where
 
     fn schedule_gui(&self, task: T) -> bool {
         if self.is_main_thread() {
-            self.executor.execute(task, true);
+            match self.executor.upgrade() {
+                Some(executor) => executor.execute(task, true),
+                None => {
+                    nih_debug_assert_failure!("GUI task was posted after the executor was dropped")
+                }
+            }
+
             true
         } else {
             self.background_thread.schedule(task)

src/event_loop/macos.rs

@@ -9,7 +9,7 @@ use core_foundation::runloop::{
 use crossbeam::channel::{self, Receiver, Sender};
 use objc::{class, msg_send, sel, sel_impl};
 use std::os::raw::c_void;
-use std::sync::Arc;
+use std::sync::Weak;
 
 use super::{BackgroundThread, EventLoop, MainThreadExecutor};
 
@@ -24,7 +24,7 @@ pub(crate) struct MacOSEventLoop<T, E> {
     /// The thing that ends up executing these tasks. The tasks are usually executed from the worker
     /// thread, but if the current thread is the main thread then the task cna also be executed
     /// directly.
-    executor: Arc<E>,
+    executor: Weak<E>,
 
     /// A background thread for running tasks independently from the host's GUI thread. Useful for
     /// longer, blocking tasks.
@@ -40,7 +40,7 @@ pub(crate) struct MacOSEventLoop<T, E> {
     /// The data that is passed to the external run loop source callback function via a raw pointer.
     /// The data is not accessed from the Rust side after creating it but it's kept here so as not
     /// to get dropped.
-    _callback_data: Box<(Arc<E>, Receiver<T>)>,
+    _callback_data: Box<(Weak<E>, Receiver<T>)>,
 }
 
 impl<T, E> EventLoop<T, E> for MacOSEventLoop<T, E>
@@ -48,7 +48,7 @@ where
     T: Send + 'static,
     E: MainThreadExecutor<T> + 'static,
 {
-    fn new_and_spawn(executor: Arc<E>) -> Self {
+    fn new_and_spawn(executor: Weak<E>) -> Self {
         let (main_thread_sender, main_thread_receiver) =
             channel::bounded::<T>(super::TASK_QUEUE_CAPACITY);
 
@@ -88,7 +88,11 @@ where
 
     fn schedule_gui(&self, task: T) -> bool {
         if self.is_main_thread() {
-            self.executor.execute(task, true);
+            match self.executor.upgrade() {
+                Some(executor) => executor.execute(task, true),
+                None => nih_debug_assert_failure!("GUI task posted after the executor was dropped"),
+            }
+
             true
         } else {
             // Only signal the main thread callback to be called if the task was added to the queue.
@@ -131,7 +135,15 @@ where
     T: Send + 'static,
     E: MainThreadExecutor<T> + 'static,
 {
-    let (executor, receiver) = unsafe { &*(info as *mut (Arc<E>, Receiver<T>)) };
+    let (executor, receiver) = unsafe { &*(info as *mut (Weak<E>, Receiver<T>)) };
+    let executor = match executor.upgrade() {
+        Some(executor) => executor,
+        None => {
+            nih_debug_assert_failure!("GUI task was posted after the executor was dropped");
+            return;
+        }
+    };
+
     while let Ok(task) = receiver.try_recv() {
         executor.execute(task, true);
     }

src/event_loop/windows.rs

@@ -5,7 +5,7 @@ use crossbeam::channel;
 use std::ffi::{c_void, CString};
 use std::mem;
 use std::ptr;
-use std::sync::Arc;
+use std::sync::Weak;
 use std::thread::{self, ThreadId};
 use windows::Win32::Foundation::{HINSTANCE, HWND, LPARAM, LRESULT, PSTR, WPARAM};
 use windows::Win32::System::{
@@ -37,7 +37,7 @@ pub(crate) struct WindowsEventLoop<T, E> {
     /// The thing that ends up executing these tasks. The tasks are usually executed from the worker
     /// thread, but if the current thread is the main thread then the task cna also be executed
     /// directly.
-    executor: Arc<E>,
+    executor: Weak<E>,
 
     /// The ID of the main thread. In practice this is the ID of the thread that created this task
     /// queue.
@@ -63,7 +63,7 @@ where
     T: Send + 'static,
     E: MainThreadExecutor<T> + 'static,
 {
-    fn new_and_spawn(executor: Arc<E>) -> Self {
+    fn new_and_spawn(executor: Weak<E>) -> Self {
         let (tasks_sender, tasks_receiver) = channel::bounded(super::TASK_QUEUE_CAPACITY);
 
         // Window classes need to have unique names or else multiple plugins loaded into the same
@@ -87,8 +87,7 @@ where
         // can't pass the tasks queue and the executor to it directly, so this is a simple type
         // erased version of the polling loop.
         let callback: PollCallback = {
-            let executor = Arc::downgrade(&executor);
+            let executor = executor.clone();
-
             Box::new(move || {
                 let executor = match executor.upgrade() {
                     Some(e) => e,
@@ -137,7 +136,13 @@ where
 
     fn schedule_gui(&self, task: T) -> bool {
         if self.is_main_thread() {
-            self.executor.execute(task, true);
+            match self.executor.upgrade() {
+                Some(executor) => executor.execute(task, true),
+                None => {
+                    nih_debug_assert_failure!("GUI task was posted after the executor was dropped")
+                }
+            }
+
             true
         } else {
             let success = self.tasks_sender.try_send(task).is_ok();

src/wrapper/clap/wrapper.rs

@@ -326,7 +326,7 @@ pub enum OutputParamEvent {
 /// Because CLAP has this [`clap_host::request_host_callback()`] function, we don't need to use
 /// `OsEventLoop` and can instead just request a main thread callback directly.
 impl<P: ClapPlugin> EventLoop<Task<P>, Wrapper<P>> for Wrapper<P> {
-    fn new_and_spawn(_executor: std::sync::Arc<Self>) -> Self {
+    fn new_and_spawn(_executor: Weak<Self>) -> Self {
         panic!("What are you doing");
     }
 
@@ -713,7 +713,7 @@ impl<P: ClapPlugin> Wrapper<P> {
 
         // Same with the background thread
         *wrapper.background_thread.borrow_mut() =
-            Some(BackgroundThread::get_or_create(wrapper.clone()));
+            Some(BackgroundThread::get_or_create(Arc::downgrade(&wrapper)));
 
         wrapper
     }

src/wrapper/standalone/wrapper.rs

@@ -248,7 +248,8 @@ impl<P: Plugin, B: Backend<P>> Wrapper<P, B> {
             updated_state_receiver,
         });
 
-        *wrapper.event_loop.borrow_mut() = Some(OsEventLoop::new_and_spawn(wrapper.clone()));
+        *wrapper.event_loop.borrow_mut() =
+            Some(OsEventLoop::new_and_spawn(Arc::downgrade(&wrapper)));
 
         // The editor needs to be initialized later so the Async executor can work.
         *wrapper.editor.borrow_mut() = wrapper

src/wrapper/vst3/inner.rs

@@ -337,7 +337,8 @@ impl<P: Vst3Plugin> WrapperInner<P> {
         // FIXME: Right now this is safe, but if we are going to have a singleton main thread queue
         //        serving multiple plugin instances, Arc can't be used because its reference count
         //        is separate from the internal COM-style reference count.
-        *wrapper.event_loop.borrow_mut() = Some(OsEventLoop::new_and_spawn(wrapper.clone()));
+        *wrapper.event_loop.borrow_mut() =
+            Some(OsEventLoop::new_and_spawn(Arc::downgrade(&wrapper)));
 
         // The editor also needs to be initialized later so the Async executor can work.
         *wrapper.editor.borrow_mut() = wrapper