From 2d7829a693994b6c39ba568a63b3b9df7d39e546 Mon Sep 17 00:00:00 2001 From: Nagata Parama Aptana <61454123+lifers@users.noreply.github.com> Date: Sun, 11 Aug 2024 22:28:20 -0400 Subject: [PATCH 01/17] use RwLock for interior mutability No deadlock reasoning: - RwLock can block when reader and writer threads are contesting access - When swap lock is acquired, we either have: - an exclusive write access from one of add, remove, or clear - a shared read access from call and one of add, remove, or clear - Otherwise, - when a change lock is acquired, we have a single read access from one of add, remove, or clear - when no lock is acquired, there is no access - Therefore delegates shouldn't deadlock --- crates/libs/core/src/event.rs | 54 +++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/crates/libs/core/src/event.rs b/crates/libs/core/src/event.rs index 18211f9c1b..3344b27c37 100644 --- a/crates/libs/core/src/event.rs +++ b/crates/libs/core/src/event.rs @@ -3,7 +3,7 @@ use core::ffi::c_void; use core::marker::PhantomData; use core::mem::{size_of, transmute_copy}; use core::ptr::null_mut; -use std::sync::Mutex; +use std::sync::{Mutex, RwLock}; /// A type that you can use to declare and implement an event of a specified delegate type. /// @@ -12,7 +12,7 @@ use std::sync::Mutex; pub struct Event { swap: Mutex<()>, change: Mutex<()>, - delegates: Array, + delegates: RwLock>, } impl Default for Event { @@ -25,47 +25,51 @@ impl Event { /// Creates a new, empty `Event`. pub fn new() -> Self { Self { - delegates: Array::new(), + delegates: RwLock::new(Array::new()), swap: Mutex::default(), change: Mutex::default(), } } /// Registers a delegate with the event object. - pub fn add(&mut self, delegate: &T) -> Result { + pub fn add(&self, delegate: &T) -> Result { let mut _lock_free_drop = Array::new(); Ok({ let _change_lock = self.change.lock().unwrap(); - let mut new_delegates = Array::with_capacity(self.delegates.len() + 1)?; - for delegate in self.delegates.as_slice() { + let delegate_array = self.delegates.read().unwrap(); + let mut new_delegates = Array::with_capacity(delegate_array.len() + 1)?; + for delegate in delegate_array.as_slice() { new_delegates.push(delegate.clone()); } + drop(delegate_array); let delegate = Delegate::new(delegate)?; let token = delegate.to_token(); new_delegates.push(delegate); let _swap_lock = self.swap.lock().unwrap(); - _lock_free_drop = self.delegates.swap(new_delegates); + let mut delegate_array = self.delegates.write().unwrap(); + _lock_free_drop = delegate_array.swap(new_delegates); token }) } /// Revokes a delegate's registration from the event object. - pub fn remove(&mut self, token: i64) -> Result<()> { + pub fn remove(&self, token: i64) -> Result<()> { let mut _lock_free_drop = Array::new(); { let _change_lock = self.change.lock().unwrap(); - if self.delegates.is_empty() { + let delegate_array = self.delegates.read().unwrap(); + if delegate_array.is_empty() { return Ok(()); } - let mut capacity = self.delegates.len() - 1; + let mut capacity = delegate_array.len() - 1; let mut new_delegates = Array::new(); let mut removed = false; if capacity == 0 { - removed = self.delegates.as_slice()[0].to_token() == token; + removed = delegate_array.as_slice()[0].to_token() == token; } else { new_delegates = Array::with_capacity(capacity)?; - for delegate in self.delegates.as_slice() { + for delegate in delegate_array.as_slice() { if !removed && delegate.to_token() == token { removed = true; continue; @@ -77,32 +81,38 @@ impl Event { capacity -= 1; } } + drop(delegate_array); if removed { let _swap_lock = self.swap.lock().unwrap(); - _lock_free_drop = self.delegates.swap(new_delegates); + let mut delegate_array = self.delegates.write().unwrap(); + _lock_free_drop = delegate_array.swap(new_delegates); } } Ok(()) } /// Clears the event, removing all delegates. - pub fn clear(&mut self) { + pub fn clear(&self) { let mut _lock_free_drop = Array::new(); { let _change_lock = self.change.lock().unwrap(); - if self.delegates.is_empty() { - return; + { + let delegate_array = self.delegates.read().unwrap(); + if delegate_array.is_empty() { + return; + } } let _swap_lock = self.swap.lock().unwrap(); - _lock_free_drop = self.delegates.swap(Array::new()); + let mut delegate_array = self.delegates.write().unwrap(); + _lock_free_drop = delegate_array.swap(Array::new()); } } /// Invokes all of the event object's registered delegates with the provided callback. - pub fn call Result<()>>(&mut self, mut callback: F) -> Result<()> { + pub fn call Result<()>>(&self, mut callback: F) -> Result<()> { let lock_free_calls = { let _swap_lock = self.swap.lock().unwrap(); - self.delegates.clone() + self.delegates.read().unwrap().clone() }; for delegate in lock_free_calls.as_slice() { if let Err(error) = delegate.call(&mut callback) { @@ -123,7 +133,6 @@ impl Event { struct Array { buffer: *mut Buffer, len: usize, - _phantom: PhantomData, } impl Default for Array { @@ -138,7 +147,6 @@ impl Array { Self { buffer: null_mut(), len: 0, - _phantom: PhantomData, } } @@ -147,13 +155,12 @@ impl Array { Ok(Self { buffer: Buffer::new(capacity)?, len: 0, - _phantom: PhantomData, }) } /// Swaps the contents of two `Array` objects. fn swap(&mut self, mut other: Self) -> Self { - unsafe { core::ptr::swap(&mut self.buffer, &mut other.buffer) }; + unsafe { core::ptr::swap(self.buffer, other.buffer) }; core::mem::swap(&mut self.len, &mut other.len); other } @@ -203,7 +210,6 @@ impl Clone for Array { Self { buffer: self.buffer, len: self.len, - _phantom: PhantomData, } } } From 09a4de9a5a7c0716568d4d704ff82773b83882f1 Mon Sep 17 00:00:00 2001 From: Nagata Parama Aptana <61454123+lifers@users.noreply.github.com> Date: Sun, 11 Aug 2024 22:44:57 -0400 Subject: [PATCH 02/17] update tests to use non-mutable event --- crates/tests/event/tests/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/tests/event/tests/tests.rs b/crates/tests/event/tests/tests.rs index fe27dc2530..cfb1596907 100644 --- a/crates/tests/event/tests/tests.rs +++ b/crates/tests/event/tests/tests.rs @@ -4,7 +4,7 @@ use windows::{core::*, Foundation::*}; #[test] fn add_remove() -> Result<()> { - let mut event = Event::>::new(); + let event = Event::>::new(); // Remove a bogus event handler from an empty event source. event.remove(-123)?; @@ -39,7 +39,7 @@ fn add_remove() -> Result<()> { #[test] fn multiple() -> Result<()> { - let mut event = Event::>::new(); + let event = Event::>::new(); let a_check = Arc::new(AtomicI32::new(0)); let a_check_sender = a_check.clone(); From 0337f170e38a37a612651b542ec4c0052506b14a Mon Sep 17 00:00:00 2001 From: Nagata Parama Aptana <61454123+lifers@users.noreply.github.com> Date: Mon, 12 Aug 2024 05:57:18 -0400 Subject: [PATCH 03/17] revert accidental change on Array::swap --- crates/libs/core/src/event.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/libs/core/src/event.rs b/crates/libs/core/src/event.rs index 3344b27c37..d07fb14836 100644 --- a/crates/libs/core/src/event.rs +++ b/crates/libs/core/src/event.rs @@ -160,7 +160,7 @@ impl Array { /// Swaps the contents of two `Array` objects. fn swap(&mut self, mut other: Self) -> Self { - unsafe { core::ptr::swap(self.buffer, other.buffer) }; + unsafe { core::ptr::swap(&mut self.buffer, &mut other.buffer) }; core::mem::swap(&mut self.len, &mut other.len); other } From 33d8896279f5781ee7b54772864eb8be4d9d033c Mon Sep 17 00:00:00 2001 From: Nagata Parama Aptana <61454123+lifers@users.noreply.github.com> Date: Mon, 12 Aug 2024 21:17:44 -0400 Subject: [PATCH 04/17] use cast to *mut Array Assuming we have an exclusive access to the array --- crates/libs/core/src/event.rs | 59 ++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 29 deletions(-) diff --git a/crates/libs/core/src/event.rs b/crates/libs/core/src/event.rs index d07fb14836..a0dc3a5936 100644 --- a/crates/libs/core/src/event.rs +++ b/crates/libs/core/src/event.rs @@ -3,7 +3,7 @@ use core::ffi::c_void; use core::marker::PhantomData; use core::mem::{size_of, transmute_copy}; use core::ptr::null_mut; -use std::sync::{Mutex, RwLock}; +use std::sync::Mutex; /// A type that you can use to declare and implement an event of a specified delegate type. /// @@ -12,7 +12,7 @@ use std::sync::{Mutex, RwLock}; pub struct Event { swap: Mutex<()>, change: Mutex<()>, - delegates: RwLock>, + delegates: Array, } impl Default for Event { @@ -25,7 +25,7 @@ impl Event { /// Creates a new, empty `Event`. pub fn new() -> Self { Self { - delegates: RwLock::new(Array::new()), + delegates: Array::new(), swap: Mutex::default(), change: Mutex::default(), } @@ -36,19 +36,19 @@ impl Event { let mut _lock_free_drop = Array::new(); Ok({ let _change_lock = self.change.lock().unwrap(); - let delegate_array = self.delegates.read().unwrap(); - let mut new_delegates = Array::with_capacity(delegate_array.len() + 1)?; - for delegate in delegate_array.as_slice() { + let mut new_delegates = Array::with_capacity(self.delegates.len() + 1)?; + for delegate in self.delegates.as_slice() { new_delegates.push(delegate.clone()); } - drop(delegate_array); let delegate = Delegate::new(delegate)?; let token = delegate.to_token(); new_delegates.push(delegate); let _swap_lock = self.swap.lock().unwrap(); - let mut delegate_array = self.delegates.write().unwrap(); - _lock_free_drop = delegate_array.swap(new_delegates); + // Safety: we have exclusive access to self.delegates at this point + _lock_free_drop = unsafe { + Array::swap(core::ptr::addr_of!(self.delegates) as *mut _, new_delegates) + }; token }) } @@ -58,18 +58,17 @@ impl Event { let mut _lock_free_drop = Array::new(); { let _change_lock = self.change.lock().unwrap(); - let delegate_array = self.delegates.read().unwrap(); - if delegate_array.is_empty() { + if self.delegates.is_empty() { return Ok(()); } - let mut capacity = delegate_array.len() - 1; + let mut capacity = self.delegates.len() - 1; let mut new_delegates = Array::new(); let mut removed = false; if capacity == 0 { - removed = delegate_array.as_slice()[0].to_token() == token; + removed = self.delegates.as_slice()[0].to_token() == token; } else { new_delegates = Array::with_capacity(capacity)?; - for delegate in delegate_array.as_slice() { + for delegate in self.delegates.as_slice() { if !removed && delegate.to_token() == token { removed = true; continue; @@ -81,11 +80,12 @@ impl Event { capacity -= 1; } } - drop(delegate_array); if removed { let _swap_lock = self.swap.lock().unwrap(); - let mut delegate_array = self.delegates.write().unwrap(); - _lock_free_drop = delegate_array.swap(new_delegates); + // Safety: we have exclusive access to self.delegates at this point + _lock_free_drop = unsafe { + Array::swap(core::ptr::addr_of!(self.delegates) as *mut _, new_delegates) + }; } } Ok(()) @@ -93,18 +93,16 @@ impl Event { /// Clears the event, removing all delegates. pub fn clear(&self) { - let mut _lock_free_drop = Array::new(); + let mut _lock_free_drop = Array::::new(); { let _change_lock = self.change.lock().unwrap(); - { - let delegate_array = self.delegates.read().unwrap(); - if delegate_array.is_empty() { - return; - } + if self.delegates.is_empty() { + return; } let _swap_lock = self.swap.lock().unwrap(); - let mut delegate_array = self.delegates.write().unwrap(); - _lock_free_drop = delegate_array.swap(Array::new()); + // Safety: we have exclusive access to self.delegates at this point + _lock_free_drop = + unsafe { Array::swap(core::ptr::addr_of!(self.delegates) as *mut _, Array::new()) }; } } @@ -112,7 +110,7 @@ impl Event { pub fn call Result<()>>(&self, mut callback: F) -> Result<()> { let lock_free_calls = { let _swap_lock = self.swap.lock().unwrap(); - self.delegates.read().unwrap().clone() + self.delegates.clone() }; for delegate in lock_free_calls.as_slice() { if let Err(error) = delegate.call(&mut callback) { @@ -159,9 +157,12 @@ impl Array { } /// Swaps the contents of two `Array` objects. - fn swap(&mut self, mut other: Self) -> Self { - unsafe { core::ptr::swap(&mut self.buffer, &mut other.buffer) }; - core::mem::swap(&mut self.len, &mut other.len); + /// # Safety + /// - `current` and `other` must be valid pointers to `Array` objects. + /// - Caller must ensure that they have exclusive access to `current` and `other`. + unsafe fn swap(current: *mut Self, mut other: Self) -> Self { + core::mem::swap(&mut (*current).buffer, &mut other.buffer); + core::ptr::swap(&mut (*current).len, &mut other.len); other } From db90add830d4be81950ccd81da7f20f79f45a32a Mon Sep 17 00:00:00 2001 From: Nagata Parama Aptana <61454123+lifers@users.noreply.github.com> Date: Mon, 12 Aug 2024 21:17:44 -0400 Subject: [PATCH 05/17] use cast to *mut Array Assuming we have an exclusive access to the array --- crates/libs/core/src/event.rs | 59 ++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 29 deletions(-) diff --git a/crates/libs/core/src/event.rs b/crates/libs/core/src/event.rs index d07fb14836..a0dc3a5936 100644 --- a/crates/libs/core/src/event.rs +++ b/crates/libs/core/src/event.rs @@ -3,7 +3,7 @@ use core::ffi::c_void; use core::marker::PhantomData; use core::mem::{size_of, transmute_copy}; use core::ptr::null_mut; -use std::sync::{Mutex, RwLock}; +use std::sync::Mutex; /// A type that you can use to declare and implement an event of a specified delegate type. /// @@ -12,7 +12,7 @@ use std::sync::{Mutex, RwLock}; pub struct Event { swap: Mutex<()>, change: Mutex<()>, - delegates: RwLock>, + delegates: Array, } impl Default for Event { @@ -25,7 +25,7 @@ impl Event { /// Creates a new, empty `Event`. pub fn new() -> Self { Self { - delegates: RwLock::new(Array::new()), + delegates: Array::new(), swap: Mutex::default(), change: Mutex::default(), } @@ -36,19 +36,19 @@ impl Event { let mut _lock_free_drop = Array::new(); Ok({ let _change_lock = self.change.lock().unwrap(); - let delegate_array = self.delegates.read().unwrap(); - let mut new_delegates = Array::with_capacity(delegate_array.len() + 1)?; - for delegate in delegate_array.as_slice() { + let mut new_delegates = Array::with_capacity(self.delegates.len() + 1)?; + for delegate in self.delegates.as_slice() { new_delegates.push(delegate.clone()); } - drop(delegate_array); let delegate = Delegate::new(delegate)?; let token = delegate.to_token(); new_delegates.push(delegate); let _swap_lock = self.swap.lock().unwrap(); - let mut delegate_array = self.delegates.write().unwrap(); - _lock_free_drop = delegate_array.swap(new_delegates); + // Safety: we have exclusive access to self.delegates at this point + _lock_free_drop = unsafe { + Array::swap(core::ptr::addr_of!(self.delegates) as *mut _, new_delegates) + }; token }) } @@ -58,18 +58,17 @@ impl Event { let mut _lock_free_drop = Array::new(); { let _change_lock = self.change.lock().unwrap(); - let delegate_array = self.delegates.read().unwrap(); - if delegate_array.is_empty() { + if self.delegates.is_empty() { return Ok(()); } - let mut capacity = delegate_array.len() - 1; + let mut capacity = self.delegates.len() - 1; let mut new_delegates = Array::new(); let mut removed = false; if capacity == 0 { - removed = delegate_array.as_slice()[0].to_token() == token; + removed = self.delegates.as_slice()[0].to_token() == token; } else { new_delegates = Array::with_capacity(capacity)?; - for delegate in delegate_array.as_slice() { + for delegate in self.delegates.as_slice() { if !removed && delegate.to_token() == token { removed = true; continue; @@ -81,11 +80,12 @@ impl Event { capacity -= 1; } } - drop(delegate_array); if removed { let _swap_lock = self.swap.lock().unwrap(); - let mut delegate_array = self.delegates.write().unwrap(); - _lock_free_drop = delegate_array.swap(new_delegates); + // Safety: we have exclusive access to self.delegates at this point + _lock_free_drop = unsafe { + Array::swap(core::ptr::addr_of!(self.delegates) as *mut _, new_delegates) + }; } } Ok(()) @@ -93,18 +93,16 @@ impl Event { /// Clears the event, removing all delegates. pub fn clear(&self) { - let mut _lock_free_drop = Array::new(); + let mut _lock_free_drop = Array::::new(); { let _change_lock = self.change.lock().unwrap(); - { - let delegate_array = self.delegates.read().unwrap(); - if delegate_array.is_empty() { - return; - } + if self.delegates.is_empty() { + return; } let _swap_lock = self.swap.lock().unwrap(); - let mut delegate_array = self.delegates.write().unwrap(); - _lock_free_drop = delegate_array.swap(Array::new()); + // Safety: we have exclusive access to self.delegates at this point + _lock_free_drop = + unsafe { Array::swap(core::ptr::addr_of!(self.delegates) as *mut _, Array::new()) }; } } @@ -112,7 +110,7 @@ impl Event { pub fn call Result<()>>(&self, mut callback: F) -> Result<()> { let lock_free_calls = { let _swap_lock = self.swap.lock().unwrap(); - self.delegates.read().unwrap().clone() + self.delegates.clone() }; for delegate in lock_free_calls.as_slice() { if let Err(error) = delegate.call(&mut callback) { @@ -159,9 +157,12 @@ impl Array { } /// Swaps the contents of two `Array` objects. - fn swap(&mut self, mut other: Self) -> Self { - unsafe { core::ptr::swap(&mut self.buffer, &mut other.buffer) }; - core::mem::swap(&mut self.len, &mut other.len); + /// # Safety + /// - `current` and `other` must be valid pointers to `Array` objects. + /// - Caller must ensure that they have exclusive access to `current` and `other`. + unsafe fn swap(current: *mut Self, mut other: Self) -> Self { + core::mem::swap(&mut (*current).buffer, &mut other.buffer); + core::ptr::swap(&mut (*current).len, &mut other.len); other } From 48024c2d7ee5ea56d59ae0c98ba0e1806566da4b Mon Sep 17 00:00:00 2001 From: Nagata Parama Aptana <61454123+lifers@users.noreply.github.com> Date: Mon, 12 Aug 2024 21:19:56 -0400 Subject: [PATCH 06/17] use cast to *mut Array Assuming we have exclusive access to the array --- crates/libs/core/src/event.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/libs/core/src/event.rs b/crates/libs/core/src/event.rs index a0dc3a5936..d044ed44a5 100644 --- a/crates/libs/core/src/event.rs +++ b/crates/libs/core/src/event.rs @@ -162,7 +162,7 @@ impl Array { /// - Caller must ensure that they have exclusive access to `current` and `other`. unsafe fn swap(current: *mut Self, mut other: Self) -> Self { core::mem::swap(&mut (*current).buffer, &mut other.buffer); - core::ptr::swap(&mut (*current).len, &mut other.len); + core::mem::swap(&mut (*current).len, &mut other.len); other } From 253a005be96ac370c7ed90ea5604561107aeef02 Mon Sep 17 00:00:00 2001 From: Nagata Parama Aptana <61454123+lifers@users.noreply.github.com> Date: Tue, 13 Aug 2024 12:51:23 -0400 Subject: [PATCH 07/17] take &mut self to swap array use core::cell::UnsafeCell. --- crates/libs/core/src/event.rs | 64 +++++++++++++++++++++-------------- 1 file changed, 39 insertions(+), 25 deletions(-) diff --git a/crates/libs/core/src/event.rs b/crates/libs/core/src/event.rs index d044ed44a5..7d127dc3ea 100644 --- a/crates/libs/core/src/event.rs +++ b/crates/libs/core/src/event.rs @@ -1,4 +1,5 @@ use super::*; +use core::cell::UnsafeCell; use core::ffi::c_void; use core::marker::PhantomData; use core::mem::{size_of, transmute_copy}; @@ -12,7 +13,7 @@ use std::sync::Mutex; pub struct Event { swap: Mutex<()>, change: Mutex<()>, - delegates: Array, + delegates: UnsafeCell>, } impl Default for Event { @@ -25,7 +26,7 @@ impl Event { /// Creates a new, empty `Event`. pub fn new() -> Self { Self { - delegates: Array::new(), + delegates: UnsafeCell::new(Array::new()), swap: Mutex::default(), change: Mutex::default(), } @@ -36,8 +37,10 @@ impl Event { let mut _lock_free_drop = Array::new(); Ok({ let _change_lock = self.change.lock().unwrap(); - let mut new_delegates = Array::with_capacity(self.delegates.len() + 1)?; - for delegate in self.delegates.as_slice() { + // Safety: there is no mutable alias to self.delegates at this point + let current_delegates = unsafe { self.get_delegates() }; + let mut new_delegates = Array::with_capacity(current_delegates.len() + 1)?; + for delegate in current_delegates.as_slice() { new_delegates.push(delegate.clone()); } let delegate = Delegate::new(delegate)?; @@ -46,9 +49,7 @@ impl Event { let _swap_lock = self.swap.lock().unwrap(); // Safety: we have exclusive access to self.delegates at this point - _lock_free_drop = unsafe { - Array::swap(core::ptr::addr_of!(self.delegates) as *mut _, new_delegates) - }; + _lock_free_drop = unsafe { self.get_delegates_mut() }.swap(new_delegates); token }) } @@ -58,17 +59,19 @@ impl Event { let mut _lock_free_drop = Array::new(); { let _change_lock = self.change.lock().unwrap(); - if self.delegates.is_empty() { + // Safety: there is no mutable alias to self.delegates at this point + let current_delegates = unsafe { self.get_delegates() }; + if current_delegates.is_empty() { return Ok(()); } - let mut capacity = self.delegates.len() - 1; + let mut capacity = current_delegates.len() - 1; let mut new_delegates = Array::new(); let mut removed = false; if capacity == 0 { - removed = self.delegates.as_slice()[0].to_token() == token; + removed = current_delegates.as_slice()[0].to_token() == token; } else { new_delegates = Array::with_capacity(capacity)?; - for delegate in self.delegates.as_slice() { + for delegate in current_delegates.as_slice() { if !removed && delegate.to_token() == token { removed = true; continue; @@ -82,10 +85,8 @@ impl Event { } if removed { let _swap_lock = self.swap.lock().unwrap(); - // Safety: we have exclusive access to self.delegates at this point - _lock_free_drop = unsafe { - Array::swap(core::ptr::addr_of!(self.delegates) as *mut _, new_delegates) - }; + // Safety: we have exclusive access to self.delegates at this point + _lock_free_drop = unsafe { self.get_delegates_mut() }.swap(new_delegates); } } Ok(()) @@ -96,13 +97,14 @@ impl Event { let mut _lock_free_drop = Array::::new(); { let _change_lock = self.change.lock().unwrap(); - if self.delegates.is_empty() { + // Safety: there is no mutable alias to self.delegates at this point + let current_delegates = unsafe { self.get_delegates() }; + if current_delegates.is_empty() { return; } let _swap_lock = self.swap.lock().unwrap(); // Safety: we have exclusive access to self.delegates at this point - _lock_free_drop = - unsafe { Array::swap(core::ptr::addr_of!(self.delegates) as *mut _, Array::new()) }; + _lock_free_drop = unsafe { self.get_delegates_mut() }.swap(Array::new()); } } @@ -110,7 +112,8 @@ impl Event { pub fn call Result<()>>(&self, mut callback: F) -> Result<()> { let lock_free_calls = { let _swap_lock = self.swap.lock().unwrap(); - self.delegates.clone() + // Safety: there is no mutable alias to self.delegates at this point + unsafe { self.get_delegates() }.clone() }; for delegate in lock_free_calls.as_slice() { if let Err(error) = delegate.call(&mut callback) { @@ -125,6 +128,20 @@ impl Event { } Ok(()) } + + /// Returns an immutable reference of the delegates member. + /// # Safety + /// The caller must ensure that there is no mutable alias to self.delegates. + unsafe fn get_delegates(&self) -> &Array { + &*self.delegates.get() + } + + /// Returns a mutable reference of the delegates member. + /// # Safety + /// The caller must ensure that they have exclusive access to self.delegates. + unsafe fn get_delegates_mut(&self) -> &mut Array { + &mut *self.delegates.get() + } } /// A thread-safe reference-counted array of delegates. @@ -157,12 +174,9 @@ impl Array { } /// Swaps the contents of two `Array` objects. - /// # Safety - /// - `current` and `other` must be valid pointers to `Array` objects. - /// - Caller must ensure that they have exclusive access to `current` and `other`. - unsafe fn swap(current: *mut Self, mut other: Self) -> Self { - core::mem::swap(&mut (*current).buffer, &mut other.buffer); - core::mem::swap(&mut (*current).len, &mut other.len); + fn swap(&mut self, mut other: Self) -> Self { + core::mem::swap(&mut self.buffer, &mut other.buffer); + core::mem::swap(&mut self.len, &mut other.len); other } From b0df0b299a4d0e55114da45de48c3b6dfd197bc5 Mon Sep 17 00:00:00 2001 From: Nagata Parama Aptana <61454123+lifers@users.noreply.github.com> Date: Tue, 13 Aug 2024 13:08:52 -0400 Subject: [PATCH 08/17] clippy lint disallowed mut_from_ref --- crates/libs/core/src/event.rs | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/crates/libs/core/src/event.rs b/crates/libs/core/src/event.rs index 7d127dc3ea..517675de84 100644 --- a/crates/libs/core/src/event.rs +++ b/crates/libs/core/src/event.rs @@ -38,7 +38,7 @@ impl Event { Ok({ let _change_lock = self.change.lock().unwrap(); // Safety: there is no mutable alias to self.delegates at this point - let current_delegates = unsafe { self.get_delegates() }; + let current_delegates = unsafe { &*self.delegates.get() }; let mut new_delegates = Array::with_capacity(current_delegates.len() + 1)?; for delegate in current_delegates.as_slice() { new_delegates.push(delegate.clone()); @@ -49,7 +49,7 @@ impl Event { let _swap_lock = self.swap.lock().unwrap(); // Safety: we have exclusive access to self.delegates at this point - _lock_free_drop = unsafe { self.get_delegates_mut() }.swap(new_delegates); + _lock_free_drop = unsafe { &mut *self.delegates.get() }.swap(new_delegates); token }) } @@ -60,7 +60,7 @@ impl Event { { let _change_lock = self.change.lock().unwrap(); // Safety: there is no mutable alias to self.delegates at this point - let current_delegates = unsafe { self.get_delegates() }; + let current_delegates = unsafe { &*self.delegates.get() }; if current_delegates.is_empty() { return Ok(()); } @@ -86,7 +86,7 @@ impl Event { if removed { let _swap_lock = self.swap.lock().unwrap(); // Safety: we have exclusive access to self.delegates at this point - _lock_free_drop = unsafe { self.get_delegates_mut() }.swap(new_delegates); + _lock_free_drop = unsafe { &mut *self.delegates.get() }.swap(new_delegates); } } Ok(()) @@ -98,13 +98,13 @@ impl Event { { let _change_lock = self.change.lock().unwrap(); // Safety: there is no mutable alias to self.delegates at this point - let current_delegates = unsafe { self.get_delegates() }; + let current_delegates = unsafe { &*self.delegates.get() }; if current_delegates.is_empty() { return; } let _swap_lock = self.swap.lock().unwrap(); // Safety: we have exclusive access to self.delegates at this point - _lock_free_drop = unsafe { self.get_delegates_mut() }.swap(Array::new()); + _lock_free_drop = unsafe { &mut *self.delegates.get() }.swap(Array::new()); } } @@ -113,7 +113,7 @@ impl Event { let lock_free_calls = { let _swap_lock = self.swap.lock().unwrap(); // Safety: there is no mutable alias to self.delegates at this point - unsafe { self.get_delegates() }.clone() + unsafe { &*self.delegates.get() }.clone() }; for delegate in lock_free_calls.as_slice() { if let Err(error) = delegate.call(&mut callback) { @@ -128,20 +128,6 @@ impl Event { } Ok(()) } - - /// Returns an immutable reference of the delegates member. - /// # Safety - /// The caller must ensure that there is no mutable alias to self.delegates. - unsafe fn get_delegates(&self) -> &Array { - &*self.delegates.get() - } - - /// Returns a mutable reference of the delegates member. - /// # Safety - /// The caller must ensure that they have exclusive access to self.delegates. - unsafe fn get_delegates_mut(&self) -> &mut Array { - &mut *self.delegates.get() - } } /// A thread-safe reference-counted array of delegates. From fdaeb152d7b358d3c57a6390b54c6301fc806861 Mon Sep 17 00:00:00 2001 From: Nagata Parama Aptana <61454123+lifers@users.noreply.github.com> Date: Tue, 13 Aug 2024 13:09:32 -0400 Subject: [PATCH 09/17] impl Send and Sync for Event --- crates/libs/core/src/event.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/libs/core/src/event.rs b/crates/libs/core/src/event.rs index 517675de84..4a85b1d495 100644 --- a/crates/libs/core/src/event.rs +++ b/crates/libs/core/src/event.rs @@ -16,6 +16,9 @@ pub struct Event { delegates: UnsafeCell>, } +unsafe impl Send for Event {} +unsafe impl Sync for Event {} + impl Default for Event { fn default() -> Self { Self::new() From 9e4a471e0af05d863c20f029b25aa8f70df8cec2 Mon Sep 17 00:00:00 2001 From: Nagata Parama Aptana <61454123+lifers@users.noreply.github.com> Date: Thu, 15 Aug 2024 14:48:33 -0400 Subject: [PATCH 10/17] Replace `Array` with Rust types The `Option` that wraps `Arc<[Delegate]>` will let us assign `None` instead of creating `Arc::new([])` with no space penalty. --- crates/libs/core/src/event.rs | 290 +++++------------------------- crates/tests/event/tests/tests.rs | 28 +-- 2 files changed, 59 insertions(+), 259 deletions(-) diff --git a/crates/libs/core/src/event.rs b/crates/libs/core/src/event.rs index 4a85b1d495..4e310d8f6b 100644 --- a/crates/libs/core/src/event.rs +++ b/crates/libs/core/src/event.rs @@ -1,262 +1,89 @@ use super::*; -use core::cell::UnsafeCell; -use core::ffi::c_void; -use core::marker::PhantomData; -use core::mem::{size_of, transmute_copy}; -use core::ptr::null_mut; -use std::sync::Mutex; +use core::mem::transmute_copy; +use std::sync::{Arc, RwLock}; /// A type that you can use to declare and implement an event of a specified delegate type. /// /// The implementation is thread-safe and designed to avoid contention between events being /// raised and delegates being added or removed. pub struct Event { - swap: Mutex<()>, - change: Mutex<()>, - delegates: UnsafeCell>, -} - -unsafe impl Send for Event {} -unsafe impl Sync for Event {} - -impl Default for Event { - fn default() -> Self { - Self::new() - } + delegates: RwLock]>>>, } impl Event { /// Creates a new, empty `Event`. pub fn new() -> Self { Self { - delegates: UnsafeCell::new(Array::new()), - swap: Mutex::default(), - change: Mutex::default(), + delegates: RwLock::new(None), } } /// Registers a delegate with the event object. pub fn add(&self, delegate: &T) -> Result { - let mut _lock_free_drop = Array::new(); - Ok({ - let _change_lock = self.change.lock().unwrap(); - // Safety: there is no mutable alias to self.delegates at this point - let current_delegates = unsafe { &*self.delegates.get() }; - let mut new_delegates = Array::with_capacity(current_delegates.len() + 1)?; - for delegate in current_delegates.as_slice() { - new_delegates.push(delegate.clone()); - } - let delegate = Delegate::new(delegate)?; - let token = delegate.to_token(); - new_delegates.push(delegate); + let mut guard = self.delegates.write().unwrap(); + let mut new_list = if let Some(old_delegates) = guard.as_ref() { + let mut new_list = Vec::with_capacity(old_delegates.len() + 1); + new_list.extend_from_slice(&old_delegates); + new_list + } else { + Vec::with_capacity(1) + }; + + let new_delegate = Delegate::new(delegate)?; + let token = new_delegate.to_token(); + new_list.push(new_delegate); - let _swap_lock = self.swap.lock().unwrap(); - // Safety: we have exclusive access to self.delegates at this point - _lock_free_drop = unsafe { &mut *self.delegates.get() }.swap(new_delegates); - token - }) + *guard = Some(new_list.into()); + Ok(token) } /// Revokes a delegate's registration from the event object. - pub fn remove(&self, token: i64) -> Result<()> { - let mut _lock_free_drop = Array::new(); - { - let _change_lock = self.change.lock().unwrap(); - // Safety: there is no mutable alias to self.delegates at this point - let current_delegates = unsafe { &*self.delegates.get() }; - if current_delegates.is_empty() { - return Ok(()); - } - let mut capacity = current_delegates.len() - 1; - let mut new_delegates = Array::new(); - let mut removed = false; - if capacity == 0 { - removed = current_delegates.as_slice()[0].to_token() == token; - } else { - new_delegates = Array::with_capacity(capacity)?; - for delegate in current_delegates.as_slice() { - if !removed && delegate.to_token() == token { - removed = true; - continue; - } - if capacity == 0 { - break; - } - new_delegates.push(delegate.clone()); - capacity -= 1; - } - } - if removed { - let _swap_lock = self.swap.lock().unwrap(); - // Safety: we have exclusive access to self.delegates at this point - _lock_free_drop = unsafe { &mut *self.delegates.get() }.swap(new_delegates); + pub fn remove(&self, token: i64) { + let mut guard = self.delegates.write().unwrap(); + if let Some(old_delegates) = guard.as_ref() { + // `self.delegates` is only modified if the token is found. + if let Some(i) = old_delegates + .iter() + .position(|old_delegate| old_delegate.to_token() == token) + { + let mut new_list = Vec::with_capacity(old_delegates.len() - 1); + new_list.extend_from_slice(&old_delegates[..i]); + new_list.extend_from_slice(&old_delegates[i + 1..]); + *guard = Some(new_list.into()); } } - Ok(()) } /// Clears the event, removing all delegates. pub fn clear(&self) { - let mut _lock_free_drop = Array::::new(); - { - let _change_lock = self.change.lock().unwrap(); - // Safety: there is no mutable alias to self.delegates at this point - let current_delegates = unsafe { &*self.delegates.get() }; - if current_delegates.is_empty() { - return; - } - let _swap_lock = self.swap.lock().unwrap(); - // Safety: we have exclusive access to self.delegates at this point - _lock_free_drop = unsafe { &mut *self.delegates.get() }.swap(Array::new()); - } + let mut guard = self.delegates.write().unwrap(); + *guard = None; } /// Invokes all of the event object's registered delegates with the provided callback. - pub fn call Result<()>>(&self, mut callback: F) -> Result<()> { - let lock_free_calls = { - let _swap_lock = self.swap.lock().unwrap(); - // Safety: there is no mutable alias to self.delegates at this point - unsafe { &*self.delegates.get() }.clone() + pub fn call Result<()>>(&self, mut callback: F) { + let delegates = { + let guard = self.delegates.read().unwrap(); + if let Some(delegates) = guard.as_ref() { + delegates.clone() + } else { + // No delegates to call. + return; + } + // <-- lock is released here }; - for delegate in lock_free_calls.as_slice() { + + for delegate in delegates.iter() { if let Err(error) = delegate.call(&mut callback) { const RPC_E_SERVER_UNAVAILABLE: HRESULT = HRESULT(-2147023174); // HRESULT_FROM_WIN32(RPC_S_SERVER_UNAVAILABLE) if matches!( error.code(), imp::RPC_E_DISCONNECTED | imp::JSCRIPT_E_CANTEXECUTE | RPC_E_SERVER_UNAVAILABLE ) { - self.remove(delegate.to_token())?; + self.remove(delegate.to_token()); } } } - Ok(()) - } -} - -/// A thread-safe reference-counted array of delegates. -struct Array { - buffer: *mut Buffer, - len: usize, -} - -impl Default for Array { - fn default() -> Self { - Self::new() - } -} - -impl Array { - /// Creates a new, empty `Array` with no capacity. - fn new() -> Self { - Self { - buffer: null_mut(), - len: 0, - } - } - - /// Creates a new, empty `Array` with the specified capacity. - fn with_capacity(capacity: usize) -> Result { - Ok(Self { - buffer: Buffer::new(capacity)?, - len: 0, - }) - } - - /// Swaps the contents of two `Array` objects. - fn swap(&mut self, mut other: Self) -> Self { - core::mem::swap(&mut self.buffer, &mut other.buffer); - core::mem::swap(&mut self.len, &mut other.len); - other - } - - /// Returns `true` if the array contains no delegates. - fn is_empty(&self) -> bool { - self.len == 0 - } - - /// Returns the number of delegates in the array. - fn len(&self) -> usize { - self.len - } - - /// Appends a delegate to the back of the array. - fn push(&mut self, delegate: Delegate) { - unsafe { - (*self.buffer).as_mut_ptr().add(self.len).write(delegate); - self.len += 1; - } - } - - /// Returns a slice containing of all delegates. - fn as_slice(&self) -> &[Delegate] { - if self.is_empty() { - &[] - } else { - unsafe { core::slice::from_raw_parts((*self.buffer).as_ptr(), self.len) } - } - } - - /// Returns a mutable slice of all delegates. - fn as_mut_slice(&mut self) -> &mut [Delegate] { - if self.is_empty() { - &mut [] - } else { - unsafe { core::slice::from_raw_parts_mut((*self.buffer).as_mut_ptr(), self.len) } - } - } -} - -impl Clone for Array { - fn clone(&self) -> Self { - if !self.is_empty() { - unsafe { (*self.buffer).0.add_ref() }; - } - Self { - buffer: self.buffer, - len: self.len, - } - } -} - -impl Drop for Array { - fn drop(&mut self) { - unsafe { - if !self.is_empty() && (*self.buffer).0.release() == 0 { - core::ptr::drop_in_place(self.as_mut_slice()); - heap_free(self.buffer as _) - } - } - } -} - -/// A reference-counted buffer. -#[repr(C)] -#[repr(align(8))] -struct Buffer(imp::RefCount, PhantomData); - -impl Buffer { - /// Creates a new `Buffer` with the specified size in bytes. - fn new(len: usize) -> Result<*mut Self> { - if len == 0 { - Ok(null_mut()) - } else { - let alloc_size = size_of::() + len * size_of::>(); - let header = heap_alloc(alloc_size)? as *mut Self; - unsafe { - header.write(Self(imp::RefCount::new(1), PhantomData)); - } - Ok(header) - } - } - - /// Returns a raw pointer to the buffer's contents. The resulting pointer might be uninititalized. - fn as_ptr(&self) -> *const Delegate { - unsafe { (self as *const Self).add(1) as *const _ } - } - - /// Returns a raw mutable pointer to the buffer's contents. The resulting pointer might be uninititalized. - fn as_mut_ptr(&mut self) -> *mut Delegate { - unsafe { (self as *mut Self).add(1) as *mut _ } } } @@ -296,30 +123,3 @@ impl Delegate { } } } - -/// Allocate memory of size `bytes` using `malloc` - the `Event` implementation does not -/// need to use any particular allocator so `HeapAlloc` need not be used. -fn heap_alloc(bytes: usize) -> crate::Result<*mut c_void> { - let ptr: *mut c_void = unsafe { - extern "C" { - fn malloc(bytes: usize) -> *mut c_void; - } - - malloc(bytes) - }; - - if ptr.is_null() { - Err(Error::from_hresult(imp::E_OUTOFMEMORY)) - } else { - Ok(ptr) - } -} - -/// Free memory allocated by `heap_alloc`. -unsafe fn heap_free(ptr: *mut c_void) { - extern "C" { - fn free(ptr: *mut c_void); - } - - free(ptr); -} diff --git a/crates/tests/event/tests/tests.rs b/crates/tests/event/tests/tests.rs index cfb1596907..a9f66880bc 100644 --- a/crates/tests/event/tests/tests.rs +++ b/crates/tests/event/tests/tests.rs @@ -7,7 +7,7 @@ fn add_remove() -> Result<()> { let event = Event::>::new(); // Remove a bogus event handler from an empty event source. - event.remove(-123)?; + event.remove(-123); let check = Arc::new(AtomicI32::new(0)); let check_sender = check.clone(); @@ -19,11 +19,11 @@ fn add_remove() -> Result<()> { }))?; // Remove a bogus event handler from a non-empty event source. - event.remove(-123)?; + event.remove(-123); // Raise and observe event. assert_eq!(check.load(Ordering::Relaxed), 0); - event.call(|delegate| delegate.Invoke(None, 123))?; + event.call(|delegate| delegate.Invoke(None, 123)); assert_eq!(check.load(Ordering::Relaxed), 123); // Remove event handler. @@ -31,7 +31,7 @@ fn add_remove() -> Result<()> { // Raise event without effect. check.store(0, Ordering::Relaxed); - event.call(|delegate| delegate.Invoke(None, 123))?; + event.call(|delegate| delegate.Invoke(None, 123)); assert_eq!(check.load(Ordering::Relaxed), 0); Ok(()) @@ -51,7 +51,7 @@ fn multiple() -> Result<()> { assert_eq!(a_check.load(Ordering::Relaxed), 0); assert_eq!(b_check.load(Ordering::Relaxed), 0); assert_eq!(c_check.load(Ordering::Relaxed), 0); - event.call(|delegate| delegate.Invoke(None, 10))?; + event.call(|delegate| delegate.Invoke(None, 10)); assert_eq!(a_check.load(Ordering::Relaxed), 0); assert_eq!(b_check.load(Ordering::Relaxed), 0); assert_eq!(c_check.load(Ordering::Relaxed), 0); @@ -64,7 +64,7 @@ fn multiple() -> Result<()> { assert_eq!(a_check.load(Ordering::Relaxed), 0); assert_eq!(b_check.load(Ordering::Relaxed), 0); assert_eq!(c_check.load(Ordering::Relaxed), 0); - event.call(|delegate| delegate.Invoke(None, 10))?; + event.call(|delegate| delegate.Invoke(None, 10)); assert_eq!(a_check.load(Ordering::Relaxed), 10); assert_eq!(b_check.load(Ordering::Relaxed), 0); assert_eq!(c_check.load(Ordering::Relaxed), 0); @@ -77,7 +77,7 @@ fn multiple() -> Result<()> { assert_eq!(a_check.load(Ordering::Relaxed), 10); assert_eq!(b_check.load(Ordering::Relaxed), 0); assert_eq!(c_check.load(Ordering::Relaxed), 0); - event.call(|delegate| delegate.Invoke(None, 20))?; + event.call(|delegate| delegate.Invoke(None, 20)); assert_eq!(a_check.load(Ordering::Relaxed), 20); assert_eq!(b_check.load(Ordering::Relaxed), 20); assert_eq!(c_check.load(Ordering::Relaxed), 0); @@ -90,37 +90,37 @@ fn multiple() -> Result<()> { assert_eq!(a_check.load(Ordering::Relaxed), 20); assert_eq!(b_check.load(Ordering::Relaxed), 20); assert_eq!(c_check.load(Ordering::Relaxed), 0); - event.call(|delegate| delegate.Invoke(None, 30))?; + event.call(|delegate| delegate.Invoke(None, 30)); assert_eq!(a_check.load(Ordering::Relaxed), 30); assert_eq!(b_check.load(Ordering::Relaxed), 30); assert_eq!(c_check.load(Ordering::Relaxed), 30); - event.remove(a_token)?; + event.remove(a_token); assert_eq!(a_check.load(Ordering::Relaxed), 30); assert_eq!(b_check.load(Ordering::Relaxed), 30); assert_eq!(c_check.load(Ordering::Relaxed), 30); - event.call(|delegate| delegate.Invoke(None, 40))?; + event.call(|delegate| delegate.Invoke(None, 40)); assert_eq!(a_check.load(Ordering::Relaxed), 30); assert_eq!(b_check.load(Ordering::Relaxed), 40); assert_eq!(c_check.load(Ordering::Relaxed), 40); - event.remove(b_token)?; + event.remove(b_token); assert_eq!(a_check.load(Ordering::Relaxed), 30); assert_eq!(b_check.load(Ordering::Relaxed), 40); assert_eq!(c_check.load(Ordering::Relaxed), 40); - event.call(|delegate| delegate.Invoke(None, 50))?; + event.call(|delegate| delegate.Invoke(None, 50)); assert_eq!(a_check.load(Ordering::Relaxed), 30); assert_eq!(b_check.load(Ordering::Relaxed), 40); assert_eq!(c_check.load(Ordering::Relaxed), 50); - event.remove(c_token)?; + event.remove(c_token); assert_eq!(a_check.load(Ordering::Relaxed), 30); assert_eq!(b_check.load(Ordering::Relaxed), 40); assert_eq!(c_check.load(Ordering::Relaxed), 50); - event.call(|delegate| delegate.Invoke(None, 60))?; + event.call(|delegate| delegate.Invoke(None, 60)); assert_eq!(a_check.load(Ordering::Relaxed), 30); assert_eq!(b_check.load(Ordering::Relaxed), 40); assert_eq!(c_check.load(Ordering::Relaxed), 50); From 94493916c61efe624fd35e764aa1ca08781cf7bf Mon Sep 17 00:00:00 2001 From: Nagata Parama Aptana <61454123+lifers@users.noreply.github.com> Date: Fri, 16 Aug 2024 10:07:19 -0400 Subject: [PATCH 11/17] construct Arc directly from iterator Now we only need to allocate once for Arc --- crates/libs/core/src/event.rs | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/crates/libs/core/src/event.rs b/crates/libs/core/src/event.rs index 4e310d8f6b..f209be23df 100644 --- a/crates/libs/core/src/event.rs +++ b/crates/libs/core/src/event.rs @@ -1,5 +1,5 @@ use super::*; -use core::mem::transmute_copy; +use core::{iter::once, mem::transmute_copy}; use std::sync::{Arc, RwLock}; /// A type that you can use to declare and implement an event of a specified delegate type. @@ -21,19 +21,17 @@ impl Event { /// Registers a delegate with the event object. pub fn add(&self, delegate: &T) -> Result { let mut guard = self.delegates.write().unwrap(); - let mut new_list = if let Some(old_delegates) = guard.as_ref() { - let mut new_list = Vec::with_capacity(old_delegates.len() + 1); - new_list.extend_from_slice(&old_delegates); - new_list - } else { - Vec::with_capacity(1) - }; - let new_delegate = Delegate::new(delegate)?; let token = new_delegate.to_token(); - new_list.push(new_delegate); + let new_iter = once(new_delegate); - *guard = Some(new_list.into()); + let new_list = if let Some(old_delegates) = guard.as_ref() { + Arc::from_iter(old_delegates.iter().cloned().chain(new_iter)) + } else { + Arc::from_iter(new_iter) + }; + + *guard = Some(new_list); Ok(token) } @@ -46,10 +44,14 @@ impl Event { .iter() .position(|old_delegate| old_delegate.to_token() == token) { - let mut new_list = Vec::with_capacity(old_delegates.len() - 1); - new_list.extend_from_slice(&old_delegates[..i]); - new_list.extend_from_slice(&old_delegates[i + 1..]); - *guard = Some(new_list.into()); + let new_list = Arc::from_iter( + old_delegates[..i] + .iter() + .chain(old_delegates[i + 1..].iter()) + .cloned(), + ); + + *guard = Some(new_list); } } } From d987a51655c4d15dba7e2f5407e5a2bb0a07d1db Mon Sep 17 00:00:00 2001 From: Nagata Parama Aptana <61454123+lifers@users.noreply.github.com> Date: Fri, 16 Aug 2024 10:26:21 -0400 Subject: [PATCH 12/17] Add `Default`, `Send`, and `Sync` trait --- crates/libs/core/src/event.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/libs/core/src/event.rs b/crates/libs/core/src/event.rs index f209be23df..9b826443fe 100644 --- a/crates/libs/core/src/event.rs +++ b/crates/libs/core/src/event.rs @@ -10,6 +10,15 @@ pub struct Event { delegates: RwLock]>>>, } +unsafe impl Send for Event {} +unsafe impl Sync for Event {} + +impl Default for Event { + fn default() -> Self { + Self::new() + } +} + impl Event { /// Creates a new, empty `Event`. pub fn new() -> Self { From 520b59564849e65de90b042c2c1b11b7b96d76d8 Mon Sep 17 00:00:00 2001 From: Nagata Parama Aptana <61454123+lifers@users.noreply.github.com> Date: Fri, 16 Aug 2024 11:21:23 -0400 Subject: [PATCH 13/17] drop old delegates after lock is released --- crates/libs/core/src/event.rs | 63 +++++++++++++++++++---------------- 1 file changed, 34 insertions(+), 29 deletions(-) diff --git a/crates/libs/core/src/event.rs b/crates/libs/core/src/event.rs index 9b826443fe..534fcf0e71 100644 --- a/crates/libs/core/src/event.rs +++ b/crates/libs/core/src/event.rs @@ -29,46 +29,51 @@ impl Event { /// Registers a delegate with the event object. pub fn add(&self, delegate: &T) -> Result { - let mut guard = self.delegates.write().unwrap(); - let new_delegate = Delegate::new(delegate)?; - let token = new_delegate.to_token(); - let new_iter = once(new_delegate); - - let new_list = if let Some(old_delegates) = guard.as_ref() { - Arc::from_iter(old_delegates.iter().cloned().chain(new_iter)) - } else { - Arc::from_iter(new_iter) - }; + let mut _lock_free_drop = None; + Ok({ + let mut guard = self.delegates.write().unwrap(); + let new_delegate = Delegate::new(delegate)?; + let token = new_delegate.to_token(); + let new_iter = once(new_delegate); + + let new_list = if let Some(old_delegates) = guard.as_ref() { + Arc::from_iter(old_delegates.iter().cloned().chain(new_iter)) + } else { + Arc::from_iter(new_iter) + }; - *guard = Some(new_list); - Ok(token) + _lock_free_drop = guard.replace(new_list); + token + }) } /// Revokes a delegate's registration from the event object. pub fn remove(&self, token: i64) { - let mut guard = self.delegates.write().unwrap(); - if let Some(old_delegates) = guard.as_ref() { - // `self.delegates` is only modified if the token is found. - if let Some(i) = old_delegates - .iter() - .position(|old_delegate| old_delegate.to_token() == token) - { - let new_list = Arc::from_iter( - old_delegates[..i] - .iter() - .chain(old_delegates[i + 1..].iter()) - .cloned(), - ); - - *guard = Some(new_list); + let mut _lock_free_drop = None; + { + let mut guard = self.delegates.write().unwrap(); + if let Some(old_delegates) = guard.as_ref() { + // `self.delegates` is only modified if the token is found. + if let Some(i) = old_delegates + .iter() + .position(|old_delegate| old_delegate.to_token() == token) + { + let new_list = Arc::from_iter( + old_delegates[..i] + .iter() + .chain(old_delegates[i + 1..].iter()) + .cloned(), + ); + + _lock_free_drop = guard.replace(new_list); + } } } } /// Clears the event, removing all delegates. pub fn clear(&self) { - let mut guard = self.delegates.write().unwrap(); - *guard = None; + let _lock_free_drop = self.delegates.write().unwrap().take(); } /// Invokes all of the event object's registered delegates with the provided callback. From dff44997e41420a1f4249ad3763f6b50224312bd Mon Sep 17 00:00:00 2001 From: Nagata Parama Aptana <61454123+lifers@users.noreply.github.com> Date: Fri, 16 Aug 2024 11:40:54 -0400 Subject: [PATCH 14/17] Prefer more explicit drop order --- crates/libs/core/src/event.rs | 71 ++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 34 deletions(-) diff --git a/crates/libs/core/src/event.rs b/crates/libs/core/src/event.rs index 534fcf0e71..50eca22d1f 100644 --- a/crates/libs/core/src/event.rs +++ b/crates/libs/core/src/event.rs @@ -29,51 +29,54 @@ impl Event { /// Registers a delegate with the event object. pub fn add(&self, delegate: &T) -> Result { - let mut _lock_free_drop = None; - Ok({ - let mut guard = self.delegates.write().unwrap(); - let new_delegate = Delegate::new(delegate)?; - let token = new_delegate.to_token(); - let new_iter = once(new_delegate); - - let new_list = if let Some(old_delegates) = guard.as_ref() { - Arc::from_iter(old_delegates.iter().cloned().chain(new_iter)) - } else { - Arc::from_iter(new_iter) - }; + let mut guard = self.delegates.write().unwrap(); + let new_delegate = Delegate::new(delegate)?; + let token = new_delegate.to_token(); + let new_iter = once(new_delegate); + + let new_list = if let Some(old_delegates) = guard.as_ref() { + Arc::from_iter(old_delegates.iter().cloned().chain(new_iter)) + } else { + Arc::from_iter(new_iter) + }; + + let old_list = guard.replace(new_list); + drop(guard); + drop(old_list); // drop the old delegates _after_ releasing lock - _lock_free_drop = guard.replace(new_list); - token - }) + Ok(token) } /// Revokes a delegate's registration from the event object. pub fn remove(&self, token: i64) { - let mut _lock_free_drop = None; - { - let mut guard = self.delegates.write().unwrap(); - if let Some(old_delegates) = guard.as_ref() { - // `self.delegates` is only modified if the token is found. - if let Some(i) = old_delegates - .iter() - .position(|old_delegate| old_delegate.to_token() == token) - { - let new_list = Arc::from_iter( - old_delegates[..i] - .iter() - .chain(old_delegates[i + 1..].iter()) - .cloned(), - ); - - _lock_free_drop = guard.replace(new_list); - } + let mut guard = self.delegates.write().unwrap(); + let mut old_list = None; + if let Some(old_delegates) = guard.as_ref() { + // `self.delegates` is only modified if the token is found. + if let Some(i) = old_delegates + .iter() + .position(|old_delegate| old_delegate.to_token() == token) + { + let new_list = Arc::from_iter( + old_delegates[..i] + .iter() + .chain(old_delegates[i + 1..].iter()) + .cloned(), + ); + + old_list = guard.replace(new_list); } } + drop(guard); + drop(old_list); // drop the old delegates _after_ releasing lock } /// Clears the event, removing all delegates. pub fn clear(&self) { - let _lock_free_drop = self.delegates.write().unwrap().take(); + let mut guard = self.delegates.write().unwrap(); + let old_list = guard.take(); + drop(guard); + drop(old_list); // drop the old delegates _after_ releasing lock } /// Invokes all of the event object's registered delegates with the provided callback. From 208e52fb3bbacd31ee9c17f04cffac11621f2f03 Mon Sep 17 00:00:00 2001 From: Nagata Parama Aptana <61454123+lifers@users.noreply.github.com> Date: Fri, 16 Aug 2024 12:04:04 -0400 Subject: [PATCH 15/17] Test for `Send` and `Sync` Should fail to compile if not --- crates/tests/event/tests/tests.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/crates/tests/event/tests/tests.rs b/crates/tests/event/tests/tests.rs index a9f66880bc..079d500d8c 100644 --- a/crates/tests/event/tests/tests.rs +++ b/crates/tests/event/tests/tests.rs @@ -127,3 +127,20 @@ fn multiple() -> Result<()> { Ok(()) } + +#[test] +fn is_send_sync() -> Result<()> { + // test that the event can be sent and synced between threads + let event = Arc::new(Event::>::new()); + let event_sender = event.clone(); + + let thread = std::thread::spawn(move || { + // Nothing will happen because the event is empty. + event_sender.call(|delegate| delegate.Invoke(None, 123)); + event_sender + }); + + let returned_event = thread.join().unwrap(); + assert!(Arc::ptr_eq(&event, &returned_event)); + Ok(()) +} \ No newline at end of file From bf7295485044c0f37af620c46e0ba85bb3127b9f Mon Sep 17 00:00:00 2001 From: Nagata Parama Aptana <61454123+lifers@users.noreply.github.com> Date: Fri, 16 Aug 2024 12:07:30 -0400 Subject: [PATCH 16/17] formatting --- crates/tests/event/tests/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/tests/event/tests/tests.rs b/crates/tests/event/tests/tests.rs index 079d500d8c..06b964766c 100644 --- a/crates/tests/event/tests/tests.rs +++ b/crates/tests/event/tests/tests.rs @@ -143,4 +143,4 @@ fn is_send_sync() -> Result<()> { let returned_event = thread.join().unwrap(); assert!(Arc::ptr_eq(&event, &returned_event)); Ok(()) -} \ No newline at end of file +} From b0bc9b1888a98c99edc80f35133616c2687a5ae9 Mon Sep 17 00:00:00 2001 From: Nagata Parama Aptana <61454123+lifers@users.noreply.github.com> Date: Fri, 16 Aug 2024 12:29:54 -0400 Subject: [PATCH 17/17] Take the guard as late as possible Co-authored-by: Kenny Kerr --- crates/libs/core/src/event.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/libs/core/src/event.rs b/crates/libs/core/src/event.rs index 50eca22d1f..c04b2f8352 100644 --- a/crates/libs/core/src/event.rs +++ b/crates/libs/core/src/event.rs @@ -29,10 +29,10 @@ impl Event { /// Registers a delegate with the event object. pub fn add(&self, delegate: &T) -> Result { - let mut guard = self.delegates.write().unwrap(); let new_delegate = Delegate::new(delegate)?; let token = new_delegate.to_token(); let new_iter = once(new_delegate); + let mut guard = self.delegates.write().unwrap(); let new_list = if let Some(old_delegates) = guard.as_ref() { Arc::from_iter(old_delegates.iter().cloned().chain(new_iter))