From 3d483598078bcde3c10b8360434e3cd90a9fd79b Mon Sep 17 00:00:00 2001 From: Will Medrano Date: Thu, 29 Oct 2026 20:57:35 -0700 Subject: [PATCH] Store client reference in lifetime Arc --- CHANGELOG.md | 9 ++- src/client/async_client.rs | 113 ++++++++++++++++++++++++++++--------- src/client/callbacks.rs | 39 +++++++++---- src/client/client_impl.rs | 110 ++++++++++++++++++++++++++++-------- src/client/mod.rs | 2 +- src/lib.rs | 2 +- src/logging.rs | 22 ++++++++ src/port/port_impl.rs | 85 ++++++++++++++++------------ src/transport.rs | 44 ++++++++++----- 9 files changed, 310 insertions(+), 116 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e63eb35d3..9d463bb6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,13 @@ ## Unreleased -- Failed client activation now closes the client and frees both callback handlers instead of - leaking them ([#232](https://github.com/RustAudio/rust-jack/issues/232)). +- Ports and transport objects now hold strong references to their JACK client and remain + usable after `Client` is dropped. +- Added `ClientHandle` with an `as_ptr` method. `Port::from_raw` now takes + `Arc` instead of `Weak<()>` and no longer takes a separate client pointer. +- Callback cleanup now explicitly stops processing and detaches callbacks before freeing + their handlers, including after failed activation, even if ports or transport keep the + client open ([#232](https://github.com/RustAudio/rust-jack/issues/232)). - `Error` is now `#[non_exhaustive]`. `match`es on it outside of this crate need a wildcard arm. - Added `Error::PortUnregistrationError`. - `Client::unregister_port` now returns `Err(Error::PortUnregistrationError)` on failure instead diff --git a/src/client/async_client.rs b/src/client/async_client.rs index a31de887b..c3cddb88f 100644 --- a/src/client/async_client.rs +++ b/src/client/async_client.rs @@ -31,7 +31,7 @@ use crate::Error; /// eprintln!("Error deactivating client: {err}"); /// }; /// ``` -#[must_use = "The jack client is shut down when the AsyncClient is dropped. You most likely want to keep this alive and manually tear down with `AsyncClient::deactivate`."] +#[must_use = "Processing stops when the AsyncClient is dropped. You most likely want to keep this alive and manually tear down with `AsyncClient::deactivate`."] pub struct AsyncClient { callback: Option>>, } @@ -48,8 +48,9 @@ where /// methods specified by the `NotificationHandler` and `ProcessHandler` objects. /// /// On failure, either `Err(Error::CallbackRegistrationError)` or - /// `Err(Error::ClientActivationError)` is returned. The client is closed and both handlers - /// are dropped. + /// `Err(Error::ClientActivationError)` is returned. Callbacks are detached and both handlers + /// are dropped. Ports and transport objects may keep the JACK client open. + /// If callback cleanup also fails, the callback context is retained to keep JACK's pointers valid. /// /// `notification_handler` and `process_handler` are consumed, but they are returned when /// `Client::deactivate` is called. @@ -78,13 +79,26 @@ where callback_lock: std::sync::Mutex::new(()), }); // Declare the guard after the context so it is released before error cleanup: - // Client::drop also acquires this mutex. + // ClientHandle::drop also acquires this mutex. let _m = CREATE_OR_DESTROY_CLIENT_MUTEX.lock().ok(); - unsafe { - CallbackContext::register_callbacks(&mut callback_context)?; - } - if activate(&callback_context.client) != 0 { - return Err(Error::ClientActivationError); + let registration = unsafe { CallbackContext::register_callbacks(&mut callback_context) }; + let result = registration.and_then(|()| { + if activate(&callback_context.client) == 0 { + Ok(()) + } else { + Err(Error::ClientActivationError) + } + }); + if let Err(error) = result { + if Self::detach_callbacks(&callback_context).is_err() { + crate::logging::log_error( + b"Failed to detach callbacks after activation error. Leaking callback context.\0", + ); + // JACK may still hold pointers into the context. Retain it rather + // than free memory that a callback could still access. + Box::leak(callback_context); + } + return Err(error); } Ok(AsyncClient { callback: Some(callback_context), @@ -106,8 +120,9 @@ impl AsyncClient { /// The `handler` that was used for `Client::activate` is returned on success. Its state may /// have changed due to JACK calling its methods. /// - /// In the case of error, the `Client` is destroyed because its state is unknown, and it is - /// therefore unsafe to continue using. + /// In the case of error, the `Client` is dropped. Ports and transport objects may keep the + /// underlying JACK client open. + /// If callback cleanup fails, the callback context is retained to keep JACK's pointers valid. pub fn deactivate(self) -> Result<(Client, N, P), Error> { let mut c = self; c.maybe_deactivate() @@ -123,17 +138,16 @@ impl AsyncClient { return Err(Error::ClientIsNoLongerAlive); } let cb = self.callback.take().ok_or(Error::ClientIsNoLongerAlive)?; - drop(cb.callback_lock.lock()); - cb.is_valid_for_callback - .store(false, std::sync::atomic::Ordering::Relaxed); - // deactivate - if unsafe { j::jack_deactivate(cb.client.raw()) } != 0 { + if let Err(error) = Self::detach_callbacks(&cb) { + crate::logging::log_error( + b"Failed to detach callbacks on deactivation. Leaking callback context.\0", + ); + // Keep the callback data alive if JACK could still use it. + Box::leak(cb); drop(m); - return Err(Error::ClientDeactivationError); + return Err(error); } - // clear the callbacks - unsafe { clear_callbacks(cb.client.raw()) }?; // done, take ownership of callback if cb.has_panic.load(std::sync::atomic::Ordering::Relaxed) { drop(m); @@ -141,11 +155,23 @@ impl AsyncClient { } Ok(cb) } + + fn detach_callbacks(cb: &CallbackContext) -> Result<(), Error> { + { + let _guard = cb.callback_lock.lock(); + cb.is_valid_for_callback + .store(false, std::sync::atomic::Ordering::Relaxed); + } + if unsafe { j::jack_deactivate(cb.client.raw()) } != 0 { + return Err(Error::ClientDeactivationError); + } + unsafe { clear_callbacks(cb.client.raw()) } + } } -/// Closes the client. +/// Stops processing and releases this reference to the client. impl Drop for AsyncClient { - // Deactivate and close the client. + // Deactivate and release the client. fn drop(&mut self) { let _ = self.maybe_deactivate(); } @@ -195,8 +221,8 @@ mod tests { impl Drop for Handler { fn drop(&mut self) { - // The client must be closed before either handler is destroyed. - assert_eq!(self.port.name(), Err(Error::ClientIsNoLongerAlive)); + // Each handler's port keeps the client alive during destruction. + assert!(self.port.name().is_ok()); self.drops.fetch_add(1, Ordering::SeqCst); } } @@ -204,6 +230,8 @@ mod tests { fn check_activation_failure(start_processing: bool) { let (client, _) = Client::new("", ClientOptions::default()).unwrap(); let port = client.register_port("input", AudioIn::default()).unwrap(); + let transport = client.transport(); + let life = Arc::downgrade(&transport.client); let notification_drops = Arc::new(AtomicUsize::new(0)); let process_drops = Arc::new(AtomicUsize::new(0)); let (send, recv) = sync_channel(1); @@ -229,16 +257,49 @@ mod tests { assert_eq!(result.err(), Some(Error::ClientActivationError)); assert_eq!(notification_drops.load(Ordering::SeqCst), 1); assert_eq!(process_drops.load(Ordering::SeqCst), 1); - assert_eq!(port.name(), Err(Error::ClientIsNoLongerAlive)); + assert!(port.name().is_ok()); + // The retained client must be safe to activate after its original + // callback context and handlers have been freed. + assert_eq!(unsafe { j::jack_activate(port.client_ptr()) }, 0); + assert_eq!(unsafe { j::jack_deactivate(port.client_ptr()) }, 0); + drop(port); + assert!(life.upgrade().is_some()); + drop(transport); + assert!(life.upgrade().is_none()); } #[test] - fn activation_failure_closes_client_and_drops_handlers() { + fn activation_failure_detaches_callbacks_and_drops_handlers() { check_activation_failure(false); } #[test] - fn activation_failure_after_start_closes_client_and_drops_handlers() { + fn activation_failure_after_start_detaches_callbacks_and_drops_handlers() { check_activation_failure(true); } + + #[test] + fn dropping_async_client_detaches_callbacks_while_port_keeps_client_alive() { + let (client, _) = Client::new("", ClientOptions::default()).unwrap(); + let port = client.register_port("input", AudioIn::default()).unwrap(); + let transport = client.transport(); + let life = Arc::downgrade(&transport.client); + let drops = Arc::new(AtomicUsize::new(0)); + let handler = || Handler { + drops: drops.clone(), + port: port.clone_unowned(), + processed: None, + }; + let active = client.activate_async(handler(), handler()).unwrap(); + + drop(active); + assert_eq!(drops.load(Ordering::SeqCst), 2); + assert!(port.name().is_ok()); + assert_eq!(unsafe { j::jack_activate(port.client_ptr()) }, 0); + assert_eq!(unsafe { j::jack_deactivate(port.client_ptr()) }, 0); + + drop(port); + drop(transport); + assert!(life.upgrade().is_none()); + } } diff --git a/src/client/callbacks.rs b/src/client/callbacks.rs index 7b757f4f1..60d0d63ef 100644 --- a/src/client/callbacks.rs +++ b/src/client/callbacks.rs @@ -496,30 +496,45 @@ where } } +unsafe extern "C" fn noop_thread_init_callback(_: *mut libc::c_void) {} + /// Unsafe ffi wrapper that clears the callbacks registered to `client`. /// /// This is mostly for use within the jack crate itself. /// /// Returns `Err(Error::CallbackDeregistrationError)` on failure. /// -/// # Unsafe -/// -/// * Uses ffi calls, be careful. -/// -/// # TODO -/// -/// * Implement correctly. Freezes on my system. Maybe this makes sense now? it doesn't disturb my program +/// # Safety +/// The pointer must refer to a valid, inactive JACK client. pub unsafe fn clear_callbacks(client: *mut j::jack_client_t) -> Result<(), Error> { - j::jack_set_thread_init_callback(client, None, std::ptr::null_mut()); - j::jack_set_process_callback(client, None, std::ptr::null_mut()); - Ok(()) + let data = std::ptr::null_mut(); + j::jack_on_info_shutdown(client, None, data); + let results = [ + // jackd2 rejects a null thread init callback, so install a no-op instead. + j::jack_set_thread_init_callback(client, Some(noop_thread_init_callback), data), + j::jack_set_process_callback(client, None, data), + j::jack_set_sync_callback(client, None, data), + j::jack_set_freewheel_callback(client, None, data), + j::jack_set_buffer_size_callback(client, None, data), + j::jack_set_sample_rate_callback(client, None, data), + j::jack_set_client_registration_callback(client, None, data), + j::jack_set_port_registration_callback(client, None, data), + j::jack_set_port_connect_callback(client, None, data), + j::jack_set_graph_order_callback(client, None, data), + j::jack_set_xrun_callback(client, None, data), + ]; + if results.iter().any(|&result| result != 0) { + Err(Error::CallbackDeregistrationError) + } else { + Ok(()) + } } /// The information used by JACK to process data. pub struct CallbackContext { /// The underlying JACK client. - /// Keep this field first so the client is closed, stopping callbacks, before the handlers - /// and the rest of the context are dropped. + /// Callbacks must be stopped and detached before this context is dropped: + /// ports and transport may keep the JACK client open afterwards. pub client: Client, /// The handler for notifications. pub notification: N, diff --git a/src/client/client_impl.rs b/src/client/client_impl.rs index 342bac55e..0c3c7ac19 100644 --- a/src/client/client_impl.rs +++ b/src/client/client_impl.rs @@ -1,6 +1,6 @@ use jack_sys as j; use std::fmt::Debug; -use std::sync::Arc; +use std::sync::{Arc, Mutex}; use std::{ffi, fmt, ptr}; use crate::client::common::CREATE_OR_DESTROY_CLIENT_MUTEX; @@ -30,15 +30,31 @@ use crate::{ /// ``` pub type InternalClientID = j::jack_intclient_t; -#[allow(dead_code)] -pub struct Client( - *mut j::jack_client_t, - Arc<()>, - Option>, -); +pub struct Client(Arc); -unsafe impl Send for Client {} -unsafe impl Sync for Client {} +/// Shared ownership of an open JACK client. +/// +/// The client is closed when the last strong reference is dropped. +pub struct ClientHandle { + client_ptr: *mut j::jack_client_t, + // Keep callback storage alive until after jack_client_close returns. + property_change_handler: Mutex>>, +} + +// JACK clients support calls from multiple threads. Callback storage is only +// accessed through the mutex during registration and by JACK's callback thread. +unsafe impl Send for ClientHandle {} +unsafe impl Sync for ClientHandle {} + +impl ClientHandle { + /// Return the underlying JACK client pointer. + /// + /// Keep a strong reference to this handle alive while using the pointer. + #[inline(always)] + pub fn as_ptr(&self) -> *mut j::jack_client_t { + self.client_ptr + } +} impl Client { /// Opens a JACK client with the given name and options. If the client is successfully opened, @@ -69,7 +85,7 @@ impl Client { if client.is_null() { Err(Error::ClientError(status)) } else { - Ok((Client(client, Arc::default(), None), status)) + Ok((unsafe { Client::from_raw(client) }, status)) } } @@ -302,7 +318,7 @@ impl Client { if pp.is_null() { Err(Error::PortRegistrationError(port_name.to_string())) } else { - Ok(unsafe { Port::from_raw(port_spec, self.raw(), pp, Arc::downgrade(&self.1)) }) + Ok(unsafe { Port::from_raw(port_spec, pp, Arc::clone(&self.0)) }) } } @@ -312,7 +328,7 @@ impl Client { if pp.is_null() { None } else { - Some(unsafe { Port::from_raw(Unowned {}, self.raw(), pp, Arc::downgrade(&self.1)) }) + Some(unsafe { Port::from_raw(Unowned {}, pp, Arc::clone(&self.0)) }) } } @@ -323,7 +339,7 @@ impl Client { if pp.is_null() { None } else { - Some(unsafe { Port::from_raw(Unowned {}, self.raw(), pp, Arc::downgrade(&self.1)) }) + Some(unsafe { Port::from_raw(Unowned {}, pp, Arc::clone(&self.0)) }) } } @@ -621,7 +637,7 @@ impl Client { /// This is mostly for use within the jack crate itself. #[inline(always)] pub fn raw(&self) -> *mut j::jack_client_t { - self.0 + self.0.as_ptr() } /// Create a `Client` from an ffi pointer. @@ -631,17 +647,19 @@ impl Client { /// # Safety /// It is unsafe to create a `Client` from a raw pointer. pub unsafe fn from_raw(p: *mut j::jack_client_t) -> Self { - Client(p, Arc::default(), None) + Client(Arc::new(ClientHandle { + client_ptr: p, + property_change_handler: Mutex::new(None), + })) } /// Get a `Transport` object associated with this client. /// /// # Remarks - /// * The transport methods will only work during this client's lifetime. + /// * The transport keeps the JACK client open even after this `Client` is dropped. pub fn transport(&self) -> Transport { Transport { - client_ptr: self.0, - client_life: Arc::downgrade(&self.1), + client: Arc::clone(&self.0), } } @@ -656,10 +674,11 @@ impl Client { &mut self, handler: H, ) -> Result<(), Error> { - assert!(self.2.is_none()); + let mut property_change_handler = self.0.property_change_handler.lock().unwrap(); + assert!(property_change_handler.is_none()); let handler = Box::into_raw(Box::new(handler)); unsafe { - self.2 = Some(Box::from_raw(handler)); + *property_change_handler = Some(Box::from_raw(handler)); let res = j::jack_set_property_change_callback( self.raw(), Some(crate::properties::property_changed::), @@ -674,14 +693,14 @@ impl Client { } /// Close the client. -impl Drop for Client { +impl Drop for ClientHandle { fn drop(&mut self) { let _m = CREATE_OR_DESTROY_CLIENT_MUTEX.lock().ok(); // Rep invariant. - debug_assert!(!self.raw().is_null()); + debug_assert!(!self.client_ptr.is_null()); // Best effort close client. - let _res = unsafe { j::jack_client_close(self.raw()) }; - self.0 = ptr::null_mut(); + let _res = unsafe { j::jack_client_close(self.client_ptr) }; + self.client_ptr = ptr::null_mut(); } } @@ -794,3 +813,46 @@ pub struct CycleTimes { pub next_usecs: Time, pub period_usecs: libc::c_float, } + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::atomic::{AtomicBool, Ordering}; + + struct Handler(Arc); + + impl PropertyChangeHandler for Handler { + fn property_changed(&mut self, _: &crate::PropertyChange) {} + } + + impl Drop for Handler { + fn drop(&mut self) { + self.0.store(true, Ordering::SeqCst); + } + } + + #[test] + fn property_handler_lives_until_client_handle_closes() { + let (mut client, _) = Client::new("", Default::default()).unwrap(); + // Registration must also work after other owners have been created. + let transport = client.transport(); + let dropped = Arc::new(AtomicBool::new(false)); + client + .register_property_change_handler(Handler(dropped.clone())) + .unwrap(); + let port = client + .register_port("retained", crate::AudioIn::default()) + .unwrap(); + + drop(client); + assert!(!dropped.load(Ordering::SeqCst)); + assert!(transport.query_state().is_ok()); + + drop(transport); + assert!(!dropped.load(Ordering::SeqCst)); + assert!(port.name().is_ok()); + + drop(port); + assert!(dropped.load(Ordering::SeqCst)); + } +} diff --git a/src/client/mod.rs b/src/client/mod.rs index b337131ce..2ca24d82c 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -12,7 +12,7 @@ mod client_status; pub use self::async_client::AsyncClient; pub use self::callbacks::{NotificationHandler, ProcessHandler}; -pub use self::client_impl::{Client, CycleTimes, InternalClientID, ProcessScope}; +pub use self::client_impl::{Client, ClientHandle, CycleTimes, InternalClientID, ProcessScope}; pub use self::client_options::ClientOptions; pub use self::client_status::ClientStatus; pub use self::common::CLIENT_NAME_SIZE; diff --git a/src/lib.rs b/src/lib.rs index dc815c892..a2973dd7a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -36,7 +36,7 @@ #[allow(deprecated)] pub use crate::client::ClosureProcessHandler; pub use crate::client::{ - AsyncClient, Client, ClientOptions, ClientStatus, CycleTimes, InternalClientID, + AsyncClient, Client, ClientHandle, ClientOptions, ClientStatus, CycleTimes, InternalClientID, NotificationHandler, ProcessHandler, ProcessScope, CLIENT_NAME_SIZE, }; pub use crate::jack_enums::{Control, Error, LatencyType}; diff --git a/src/logging.rs b/src/logging.rs index 18aa90947..80fd0c03e 100644 --- a/src/logging.rs +++ b/src/logging.rs @@ -11,6 +11,25 @@ pub(crate) fn maybe_init_logging() { }); } +type LogFn = unsafe extern "C" fn(*const libc::c_char); + +/// The error function most recently registered with JACK. +static ERROR_FN: std::sync::Mutex> = std::sync::Mutex::new(None); + +/// Log an error message through the same handler JACK uses for its own errors. Does not allocate. +/// +/// `msg` must be NUL-terminated, e.g. `b"message\0"`; otherwise nothing is logged. +pub(crate) fn log_error(msg: &'static [u8]) { + if msg.last() != Some(&0) { + return; + } + maybe_init_logging(); + let error_fn = ERROR_FN.lock().ok().and_then(|f| *f); + if let Some(error_fn) = error_fn { + unsafe { error_fn(msg.as_ptr() as *const libc::c_char) }; + } +} + /// Describes how JACK should log info and error messages. #[derive(Copy, Clone, Debug)] pub enum LoggerType { @@ -66,6 +85,9 @@ fn set_logger_impl(logger: LoggerType) { LoggerType::Log => error_handler, LoggerType::Custom { error, .. } => error, }; + if let Ok(mut f) = ERROR_FN.lock() { + *f = Some(error_fn); + } unsafe { jack_sys::jack_set_error_function(Some(error_fn)); jack_sys::jack_set_info_function(Some(info_fn)); diff --git a/src/port/port_impl.rs b/src/port/port_impl.rs index fbb1b3811..cbada24c2 100644 --- a/src/port/port_impl.rs +++ b/src/port/port_impl.rs @@ -4,10 +4,10 @@ use std::cmp::{Eq, Ord, Ordering, PartialEq, PartialOrd}; use std::fmt::Debug; use std::hash::{Hash, Hasher}; use std::marker::Sized; -use std::sync::Weak; +use std::sync::Arc; use std::{ffi, fmt, iter}; -use crate::{Error, Frames, LatencyType, PortFlags}; +use crate::{ClientHandle, Error, Frames, LatencyType, PortFlags}; lazy_static! { /// The maximum string length for port names. @@ -43,12 +43,13 @@ pub unsafe trait PortSpec: Sized { /// Most JACK functionality is exposed, including the raw pointers, but it should be possible to /// create a client without the need for calling `unsafe` `Port` methods. /// +/// A port keeps its JACK client open, even after the original `Client` is dropped. +/// /// Also, ports can be compared and hashed using their raw pointers. pub struct Port { spec: PS, - client_ptr: *mut j::jack_client_t, port_ptr: *mut j::jack_port_t, - client_life: Weak<()>, + client: Arc, } unsafe impl Send for Port {} @@ -65,15 +66,13 @@ impl Port { pub fn clone_unowned(&self) -> Port { Port { spec: Unowned, - client_ptr: self.client_ptr(), port_ptr: self.raw(), - client_life: self.client_life.clone(), + client: self.client.clone(), } } /// Returns the full name of the port, including the "client_name:" prefix. pub fn name(&self) -> Result { - self.check_client_life()?; let s = unsafe { ffi::CStr::from_ptr(j::jack_port_name(self.raw())) .to_string_lossy() @@ -85,7 +84,6 @@ impl Port { /// Returns the short name of the port, it excludes the "client_name:" /// prefix. pub fn short_name(&self) -> Result { - self.check_client_life()?; let s = unsafe { ffi::CStr::from_ptr(j::jack_port_short_name(self.raw())) .to_string_lossy() @@ -104,7 +102,6 @@ impl Port { /// The port type. JACK's built in types include `"32 bit float mono audio`" and `"8 bit raw /// midi"`. Custom types may also be used. pub fn port_type(&self) -> Result { - self.check_client_life()?; let s = unsafe { ffi::CStr::from_ptr(j::jack_port_type(self.raw())) .to_string_lossy() @@ -115,7 +112,6 @@ impl Port { /// Number of ports connected to/from `&self`. pub fn connected_count(&self) -> Result { - self.check_client_life()?; let n = unsafe { j::jack_port_connected(self.raw()) }; Ok(n as usize) } @@ -123,7 +119,6 @@ impl Port { /// Returns `true` if the port is directly connected to a port with the /// name `port_name`. pub fn is_connected_to(&self, port_name: &str) -> Result { - self.check_client_life()?; let res = unsafe { let port_name = ffi::CString::new(port_name).unwrap(); j::jack_port_connected_to(self.raw(), port_name.as_ptr()) @@ -138,7 +133,6 @@ impl Port { /// /// Will return up to 2 strings. pub fn aliases(&self) -> Result, Error> { - self.check_client_life()?; let mut a: Vec = iter::repeat_n(0, *PORT_NAME_SIZE + 1).collect(); let mut b = a.clone(); unsafe { @@ -155,7 +149,6 @@ impl Port { /// Returns `true` if monitoring has been requested for `self`. pub fn is_monitoring_input(&self) -> Result { - self.check_client_life()?; match unsafe { j::jack_port_monitoring_input(self.raw()) } { 0 => Ok(false), _ => Ok(true), @@ -166,7 +159,6 @@ impl Port { /// /// This only works if the port has the `CAN_MONITOR` flag set. pub fn request_monitor(&self, enable_monitor: bool) -> Result<(), Error> { - self.check_client_life()?; let onoff = i32::from(enable_monitor); let res = unsafe { j::jack_port_request_monitor(self.raw(), onoff) }; match res { @@ -179,7 +171,6 @@ impl Port { /// off, and turns it off if only one request has been made to turn it on. Otherwise it does /// nothing. pub fn ensure_monitor(&self, enable_monitor: bool) -> Result<(), Error> { - self.check_client_life()?; let onoff = i32::from(enable_monitor); let res = unsafe { j::jack_port_ensure_monitor(self.raw(), onoff) }; match res { @@ -191,7 +182,6 @@ impl Port { /// Set's the short name of the port. If the full name is longer than `PORT_NAME_SIZE`, then it /// will be truncated. pub fn set_name(&mut self, short_name: &str) -> Result<(), Error> { - self.check_client_life()?; let short_name = ffi::CString::new(short_name).unwrap(); let res = unsafe { j::jack_port_set_name(self.raw(), short_name.as_ptr()) }; match res { @@ -211,7 +201,6 @@ impl Port { /// Ports can have up to two aliases - if both are already set, this function will return an /// error. pub fn set_alias(&mut self, alias: &str) -> Result<(), Error> { - self.check_client_life()?; let alias = ffi::CString::new(alias).unwrap(); let res = unsafe { j::jack_port_set_alias(self.raw(), alias.as_ptr()) }; match res { @@ -224,7 +213,6 @@ impl Port { /// /// After a successful call, `alias` can no longer be used as an alternate name for `self`. pub fn unset_alias(&mut self, alias: &str) -> Result<(), Error> { - self.check_client_life()?; let alias = ffi::CString::new(alias).unwrap(); let res = unsafe { j::jack_port_unset_alias(self.raw(), alias.as_ptr()) }; match res { @@ -233,32 +221,32 @@ impl Port { } } - /// Create a Port from raw JACK pointers. + /// Create a Port from a raw JACK port pointer and a client handle. /// /// This is mostly for use within the jack crate itself. /// /// # Safety - /// It is unsafe to create a `Port` from raw pointers. + /// The pointer must refer to a valid port, and `client` must be the handle + /// of the client through which the port was obtained. pub unsafe fn from_raw( spec: PS, - client_ptr: *mut j::jack_client_t, port_ptr: *mut j::jack_port_t, - client_life: Weak<()>, + client: Arc, ) -> Self { Port { spec, port_ptr, - client_ptr, - client_life, + client, } } /// Obtain the client pointer that spawned this port. /// /// This is mostly for use within the jack crate itself. + /// Keep this port or another strong reference to the client alive while using the pointer. #[inline(always)] pub fn client_ptr(&self) -> *mut j::jack_client_t { - self.client_ptr + self.client.as_ptr() } /// Obtain the ffi port pointer. @@ -279,8 +267,6 @@ impl Port { /// `.as_mut_slice()` for audio and `.midi_iter` for midi. #[inline(always)] pub unsafe fn buffer(&self, n_frames: Frames) -> *mut libc::c_void { - // We don't check for life to improve performance in a very hot codepath. - // self.check_client_life()?; j::jack_port_get_buffer(self.port_ptr, n_frames) } @@ -310,13 +296,6 @@ impl Port { unsafe { j::jack_port_get_latency_range(self.port_ptr, mode.to_ffi(), &mut ffi_range) }; (ffi_range.min, ffi_range.max) } - - fn check_client_life(&self) -> Result<(), Error> { - self.client_life - .upgrade() - .map(|_| ()) - .ok_or(Error::ClientIsNoLongerAlive) - } } impl Port { @@ -334,8 +313,9 @@ impl Port { self.get_connections() .into_iter() .map(|name| { - let (client, _port) = name.split_once(':').unwrap(); - crate::Client::uuid_of_client_by_name_raw(self.client_ptr(), client).unwrap() + let (client_name, _port) = name.split_once(':').unwrap(); + crate::Client::uuid_of_client_by_name_raw(self.client.as_ptr(), client_name) + .unwrap() }) .collect() } @@ -411,3 +391,36 @@ impl Clone for Port { self.clone_unowned() } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::{AudioIn, Client}; + + #[test] + fn ports_keep_client_alive_until_last_port_is_dropped() { + let (client, _) = Client::new("", Default::default()).unwrap(); + let port = client + .register_port("retained", AudioIn::default()) + .unwrap(); + let life = Arc::downgrade(&port.client); + let name = port.name().unwrap(); + let unowned = client.port_by_name(&name).unwrap(); + let clone = port.clone_unowned(); + let ptr = client.raw(); + + std::thread::spawn(move || drop(client)).join().unwrap(); + assert_eq!(port.name(), Ok(name.clone())); + assert_eq!(port.client_ptr(), ptr); + assert_eq!(unowned.client_ptr(), ptr); + assert_eq!(clone.client_ptr(), ptr); + + drop(port); + drop(unowned); + assert_eq!(clone.name(), Ok(name)); + assert!(life.upgrade().is_some()); + + drop(clone); + assert!(life.upgrade().is_none()); + } +} diff --git a/src/transport.rs b/src/transport.rs index ae95ad0df..9a98ae8b4 100644 --- a/src/transport.rs +++ b/src/transport.rs @@ -1,21 +1,18 @@ //! JACK transport wrappers. //! See the [transport design api docs](https://jackaudio.org/api/transport-design.html) for more info. -use crate::{Frames, Time}; +use crate::{ClientHandle, Frames, Time}; use jack_sys as j; -use std::sync::Weak; +use std::sync::Arc; pub type Result = ::std::result::Result; /// A structure for querying and manipulating the JACK transport. +/// +/// Keeps its JACK client open even after the original `Client` is dropped. pub struct Transport { - pub(crate) client_ptr: *mut j::jack_client_t, - pub(crate) client_life: Weak<()>, + pub(crate) client: Arc, } -//all exposed methods are realtime safe -unsafe impl Send for Transport {} -unsafe impl Sync for Transport {} - /// A structure representing the transport position. #[repr(transparent)] pub struct TransportPosition(j::jack_position_t); @@ -201,12 +198,8 @@ impl Transport { }) } - fn with_client R, R>(&self, func: F) -> Result { - if self.client_life.upgrade().is_some() { - Ok(func(self.client_ptr)) - } else { - Err(crate::Error::ClientIsNoLongerAlive) - } + fn with_client R, R>(&self, func: F) -> Result { + Ok(func(self.client.as_ptr())) } // Helper to create generic error from jack response @@ -480,3 +473,26 @@ impl Default for TransportBBT { } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::Client; + + #[test] + fn transport_keeps_client_alive_until_dropped() { + let (client, _) = Client::new("", Default::default()).unwrap(); + let transport = client.transport(); + let life = Arc::downgrade(&transport.client); + + std::thread::spawn(move || drop(client)).join().unwrap(); + assert_eq!(transport.stop(), Ok(())); + assert!(transport.query_state().is_ok()); + assert!(transport.query().is_ok()); + assert_eq!(transport.locate(0), Ok(())); + assert!(life.upgrade().is_some()); + + drop(transport); + assert!(life.upgrade().is_none()); + } +}