From 286226bd51796be3993d4fc1fc85fe483c62ea6a Mon Sep 17 00:00:00 2001 From: valadaptive Date: Thu, 28 Sep 2023 16:30:30 -0400 Subject: [PATCH] Run all GTK dialogs on one thread Per the GDK documentation: "GTK+, however, is not thread safe. You should only use GTK+ and GDK from the thread gtk_init() and gtk_main() were called on. This is usually referred to as the "main thread"." We were previously calling GTK functions from different threads--there was a mutex so that only one thread was calling GTK at any given time, but that's not sufficient--only the thread *that called gtk_init* is allowed to call GTK functions. To facilitate that, spin up a thread and initialize GTK on it, keeping it alive for the lifetime of the program. It's a bit unfortunate that we have to keep it around forever, but the GTK documentation seems to indicate that once you call gtk_init on a thread, you're stuck with that thread. --- src/backend/gtk3/file_dialog.rs | 32 +--- src/backend/gtk3/file_dialog/dialog_ffi.rs | 2 - src/backend/gtk3/gtk_future.rs | 60 +++---- src/backend/gtk3/message_dialog.rs | 12 +- src/backend/gtk3/utils.rs | 200 ++++++++++----------- 5 files changed, 129 insertions(+), 177 deletions(-) diff --git a/src/backend/gtk3/file_dialog.rs b/src/backend/gtk3/file_dialog.rs index 90ed7d8..bd454d5 100644 --- a/src/backend/gtk3/file_dialog.rs +++ b/src/backend/gtk3/file_dialog.rs @@ -4,7 +4,7 @@ use dialog_ffi::GtkFileDialog; use std::path::PathBuf; -use super::utils::{gtk_init_check, GTK_MUTEX}; +use super::utils::GtkGlobalThread; use crate::backend::DialogFutureType; use crate::{FileDialog, FileHandle}; @@ -17,11 +17,7 @@ use super::gtk_future::GtkDialogFuture; use crate::backend::FilePickerDialogImpl; impl FilePickerDialogImpl for FileDialog { fn pick_file(self) -> Option { - GTK_MUTEX.run_locked(|| { - if !gtk_init_check() { - return None; - }; - + GtkGlobalThread::instance().run_blocking(move || { let dialog = GtkFileDialog::build_pick_file(&self); if dialog.run() == gtk_sys::GTK_RESPONSE_ACCEPT { @@ -33,11 +29,7 @@ impl FilePickerDialogImpl for FileDialog { } fn pick_files(self) -> Option> { - GTK_MUTEX.run_locked(|| { - if !gtk_init_check() { - return None; - }; - + GtkGlobalThread::instance().run_blocking(move || { let dialog = GtkFileDialog::build_pick_files(&self); if dialog.run() == gtk_sys::GTK_RESPONSE_ACCEPT { @@ -93,11 +85,7 @@ impl AsyncFilePickerDialogImpl for FileDialog { use crate::backend::FolderPickerDialogImpl; impl FolderPickerDialogImpl for FileDialog { fn pick_folder(self) -> Option { - GTK_MUTEX.run_locked(|| { - if !gtk_init_check() { - return None; - }; - + GtkGlobalThread::instance().run_blocking(move || { let dialog = GtkFileDialog::build_pick_folder(&self); if dialog.run() == gtk_sys::GTK_RESPONSE_ACCEPT { @@ -109,11 +97,7 @@ impl FolderPickerDialogImpl for FileDialog { } fn pick_folders(self) -> Option> { - GTK_MUTEX.run_locked(|| { - if !gtk_init_check() { - return None; - }; - + GtkGlobalThread::instance().run_blocking(move || { let dialog = GtkFileDialog::build_pick_folders(&self); if dialog.run() == gtk_sys::GTK_RESPONSE_ACCEPT { @@ -169,11 +153,7 @@ impl AsyncFolderPickerDialogImpl for FileDialog { use crate::backend::FileSaveDialogImpl; impl FileSaveDialogImpl for FileDialog { fn save_file(self) -> Option { - GTK_MUTEX.run_locked(|| { - if !gtk_init_check() { - return None; - }; - + GtkGlobalThread::instance().run_blocking(move || { let dialog = GtkFileDialog::build_save_file(&self); if dialog.run() == gtk_sys::GTK_RESPONSE_ACCEPT { diff --git a/src/backend/gtk3/file_dialog/dialog_ffi.rs b/src/backend/gtk3/file_dialog/dialog_ffi.rs index 76b56a9..8c6b452 100644 --- a/src/backend/gtk3/file_dialog/dialog_ffi.rs +++ b/src/backend/gtk3/file_dialog/dialog_ffi.rs @@ -291,9 +291,7 @@ impl AsGtkDialog for GtkFileDialog { impl Drop for GtkFileDialog { fn drop(&mut self) { unsafe { - super::super::utils::wait_for_cleanup(); gtk_sys::gtk_native_dialog_destroy(self.ptr as _); - super::super::utils::wait_for_cleanup(); } } } diff --git a/src/backend/gtk3/gtk_future.rs b/src/backend/gtk3/gtk_future.rs index dbc6a9f..b0fb721 100644 --- a/src/backend/gtk3/gtk_future.rs +++ b/src/backend/gtk3/gtk_future.rs @@ -1,14 +1,10 @@ -use super::utils::GTK_EVENT_HANDLER; -use super::utils::GTK_MUTEX; +use super::utils::GtkGlobalThread; -use std::cell::RefCell; use std::pin::Pin; -use std::rc::Rc; use std::sync::{Arc, Mutex}; use std::task::{Context, Poll, Waker}; -use super::utils::gtk_init_check; use super::AsGtkDialog; struct FutureState { @@ -39,49 +35,33 @@ impl GtkDialogFuture { { let state = state.clone(); - std::thread::spawn(move || { - let request = Rc::new(RefCell::new(None)); + let callback = { + let state = state.clone(); - let callback = { - let state = state.clone(); - let request = request.clone(); - - // Callbacks are called by GTK_EVENT_HANDLER so the GTK_MUTEX is allready locked, no need to worry about that here - move |res_id| { - let mut state = state.lock().unwrap(); - - if let Some(mut dialog) = state.dialog.take() { - state.data = Some(cb(&mut dialog, res_id)); - } - - // Drop the request - request.borrow_mut().take(); + move |res_id| { + let mut state = state.lock().unwrap(); - if let Some(waker) = state.waker.take() { - waker.wake(); - } + if let Some(mut dialog) = state.dialog.take() { + state.data = Some(cb(&mut dialog, res_id)); } - }; - GTK_MUTEX.run_locked(|| { - let mut state = state.lock().unwrap(); - if gtk_init_check() { - state.dialog = Some(build()); + if let Some(waker) = state.waker.take() { + waker.wake(); } + } + }; - if let Some(dialog) = &state.dialog { - unsafe { - dialog.show(); + GtkGlobalThread::instance().run(move || { + let mut state = state.lock().unwrap(); + state.dialog = Some(build()); - let ptr = dialog.gtk_dialog_ptr(); - connect_response(ptr as *mut _, callback); - } - } else { - state.data = Some(Default::default()); - } - }); + unsafe { + let dialog = state.dialog.as_ref().unwrap(); + dialog.show(); - request.replace(Some(GTK_EVENT_HANDLER.request_iteration_start())); + let ptr = dialog.gtk_dialog_ptr(); + connect_response(ptr as *mut _, callback); + } }); } diff --git a/src/backend/gtk3/message_dialog.rs b/src/backend/gtk3/message_dialog.rs index 58c65ed..62ef923 100644 --- a/src/backend/gtk3/message_dialog.rs +++ b/src/backend/gtk3/message_dialog.rs @@ -2,7 +2,7 @@ use std::ffi::CString; use std::ptr; use super::gtk_future::GtkDialogFuture; -use super::utils::wait_for_cleanup; +use super::utils::GtkGlobalThread; use super::AsGtkDialog; use crate::message_dialog::{MessageButtons, MessageDialog, MessageLevel}; @@ -15,8 +15,6 @@ pub struct GtkMessageDialog { impl GtkMessageDialog { pub fn new(opt: MessageDialog) -> Self { - super::utils::gtk_init_check(); - let level = match opt.level { MessageLevel::Info => gtk_sys::GTK_MESSAGE_INFO, MessageLevel::Warning => gtk_sys::GTK_MESSAGE_WARNING, @@ -174,9 +172,7 @@ unsafe fn set_child_labels_selectable(dialog: *mut gtk_sys::GtkDialog) { impl Drop for GtkMessageDialog { fn drop(&mut self) { unsafe { - wait_for_cleanup(); gtk_sys::gtk_widget_destroy(self.ptr as *mut _); - wait_for_cleanup(); } } } @@ -194,8 +190,10 @@ use crate::backend::MessageDialogImpl; impl MessageDialogImpl for MessageDialog { fn show(self) -> MessageDialogResult { - let dialog = GtkMessageDialog::new(self); - dialog.run() + GtkGlobalThread::instance().run_blocking(move || { + let dialog = GtkMessageDialog::new(self); + dialog.run() + }) } } diff --git a/src/backend/gtk3/utils.rs b/src/backend/gtk3/utils.rs index aed8350..08655a4 100644 --- a/src/backend/gtk3/utils.rs +++ b/src/backend/gtk3/utils.rs @@ -1,129 +1,125 @@ use std::ptr; -use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; -use std::sync::{Arc, Mutex}; -use std::thread::JoinHandle; - -/// Ensures that gtk is allways called from one thread at the time -pub struct GtkGlobalMutex { - locker: Mutex<()>, +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::OnceLock; +use std::sync::{Arc, Condvar, Mutex}; +use std::thread::spawn; + +static GTK_THREAD: OnceLock = OnceLock::new(); + +/// GTK functions are not thread-safe, and must all be called from the thread that initialized GTK. To ensure this, we +/// spawn one thread the first time a GTK dialog is opened and keep it open for the entire lifetime of the application, +/// as GTK cannot be de-initialized or re-initialized on another thread. You're stuck on the thread on which you first +/// initialize GTK. +pub struct GtkGlobalThread { + running: Arc, } -unsafe impl Send for GtkGlobalMutex {} -unsafe impl Sync for GtkGlobalMutex {} - -impl GtkGlobalMutex { - const fn new() -> Self { - Self { - locker: Mutex::new(()), - } - } - - pub(super) fn run_locked T>(&self, cb: F) -> T { - let _guard = self.locker.lock().unwrap(); - cb() +impl GtkGlobalThread { + /// Return the global, lazily-initialized instance of the global GTK thread. + pub(super) fn instance() -> &'static Self { + GTK_THREAD.get_or_init(|| Self::new()) } -} - -pub static GTK_MUTEX: GtkGlobalMutex = GtkGlobalMutex::new(); - -/// # Event Handler -/// Counts amout of iteration requests -/// When amount of requests goes above 0 it spawns GtkThread and starts iteration -/// When amount of requests reqches 0 it stops GtkThread, and goes idle -pub struct GtkEventHandler { - thread: Mutex>, - request_count: AtomicUsize, -} -unsafe impl Send for GtkEventHandler {} -unsafe impl Sync for GtkEventHandler {} + fn new() -> Self { + // When the GtkGlobalThread is eventually dropped, we will set `running` to false and wake up the loop so + // gtk_main_iteration unblocks and we exit the thread on the next iteration. + let running = Arc::new(AtomicBool::new(true)); + let thread_running = Arc::clone(&running); + + spawn(move || { + let initialized = + unsafe { gtk_sys::gtk_init_check(ptr::null_mut(), ptr::null_mut()) == 1 }; + if !initialized { + return; + } + + loop { + if !thread_running.load(Ordering::Acquire) { + break; + } -pub static GTK_EVENT_HANDLER: GtkEventHandler = GtkEventHandler::new(); + unsafe { + gtk_sys::gtk_main_iteration(); + } + } + }); -impl GtkEventHandler { - const fn new() -> Self { - let thread = Mutex::new(None); - let request_count = AtomicUsize::new(0); Self { - thread, - request_count, + running: Arc::new(AtomicBool::new(true)), } } - /// Ask GtkEventHandler to start event iteration - /// When iteration is no longer needed, just drop IterationRequest. - /// And when numer of requests reaches 0 iteration will be stoped - pub fn request_iteration_start(&self) -> IterationRequest { - let mut thread = self.thread.lock().unwrap(); - if thread.is_none() { - thread.replace(GtkThread::new()); - } - - self.request_count.fetch_add(1, Ordering::Relaxed); - IterationRequest() - } + /// Run a function on the GTK thread, blocking on the result which is then passed back. + pub(super) fn run_blocking< + T: Send + Clone + std::fmt::Debug + 'static, + F: FnOnce() -> T + Send + 'static, + >( + &self, + cb: F, + ) -> T { + let data: Arc<(Mutex>, _)> = Arc::new((Mutex::new(None), Condvar::new())); + let thread_data = Arc::clone(&data); + let mut cb = Some(cb); + unsafe { + connect_idle(move || { + // connect_idle takes a FnMut; convert our FnOnce into that by ensuring we only call it once + let res = cb.take().expect("Callback should only be called once")(); + + // pass the result back to the main thread + let (lock, cvar) = &*thread_data; + *lock.lock().unwrap() = Some(res); + cvar.notify_all(); + + glib_sys::GFALSE + }); + }; - fn iteration_stop(&self) { - self.thread.lock().unwrap().take(); + // wait for GTK thread to execute the callback and place the result into `data` + let lock_res = data + .1 + .wait_while(data.0.lock().unwrap(), |res| res.is_none()) + .unwrap(); + lock_res.as_ref().unwrap().clone() } - fn request_iteration_stop(&self) { - self.request_count.fetch_sub(1, Ordering::Release); - - if self.request_count.load(Ordering::Acquire) == 0 { - self.iteration_stop(); - } + /// Launch a function on the GTK thread without blocking. + pub(super) fn run(&self, cb: F) { + let mut cb = Some(cb); + unsafe { + connect_idle(move || { + cb.take().expect("Callback should only be called once")(); + glib_sys::GFALSE + }); + }; } } -pub struct IterationRequest(); - -impl Drop for IterationRequest { +impl Drop for GtkGlobalThread { fn drop(&mut self) { - GTK_EVENT_HANDLER.request_iteration_stop(); + self.running.store(false, Ordering::Release); + unsafe { glib_sys::g_main_context_wakeup(std::ptr::null_mut()) }; } } -/// Thread that iterates gtk events -struct GtkThread { - _handle: JoinHandle<()>, - running: Arc, -} - -impl GtkThread { - fn new() -> Self { - let running = Arc::new(AtomicBool::new(true)); +unsafe fn connect_idle glib_sys::gboolean + Send + 'static>(f: F) { + unsafe extern "C" fn response_trampoline glib_sys::gboolean + Send + 'static>( + f: glib_sys::gpointer, + ) -> glib_sys::gboolean { + let f: &mut F = &mut *(f as *mut F); - let _handle = { - let running = running.clone(); - std::thread::spawn(move || { - while running.load(Ordering::Acquire) { - GTK_MUTEX.run_locked(|| unsafe { - while gtk_sys::gtk_events_pending() == 1 { - gtk_sys::gtk_main_iteration(); - } - }); - } - }) - }; - - Self { _handle, running } + f() } -} + let f_box: Box = Box::new(f); -impl Drop for GtkThread { - fn drop(&mut self) { - self.running.store(false, Ordering::Release); + unsafe extern "C" fn destroy_closure(ptr: *mut std::ffi::c_void) { + // destroy + let _ = Box::::from_raw(ptr as *mut _); } -} - -pub fn gtk_init_check() -> bool { - unsafe { gtk_sys::gtk_init_check(ptr::null_mut(), ptr::null_mut()) == 1 } -} -/// gtk_main_iteration() -pub unsafe fn wait_for_cleanup() { - while gtk_sys::gtk_events_pending() == 1 { - gtk_sys::gtk_main_iteration(); - } + glib_sys::g_idle_add_full( + glib_sys::G_PRIORITY_DEFAULT_IDLE, + Some(response_trampoline::), + Box::into_raw(f_box) as glib_sys::gpointer, + Some(destroy_closure::), + ); }