Add Event<T> type that can be used to implement a WinRT event
#1705
Conversation
kennykerr
changed the title
Event<T> type that can be used to declare and implement a WinRT eventEvent<T> type that can be used to implement a WinRT event
| pub fn new(object: &T) -> Result<Self> { | ||
| let unknown: &IUnknown = unsafe { std::mem::transmute(object) }; | ||
| unsafe { RoGetAgileReference(AGILEREFERENCE_DEFAULT, &T::IID, unknown).map(|reference| Self(reference, Default::default())) } |
There was a problem hiding this comment.
This just removes the redundant constraint on the AgileReference constructor. The Interface trait doesn't "know" that it requires IUnknown. That's something I'll try to fix separately.
| pub fn new() -> Self { | ||
| Self { delegates: Array::new(), swap: Mutex::default(), change: Mutex::default() } | ||
| } | ||
| /// Registers a delegate with the event object. |
There was a problem hiding this comment.
Nit: it would be nice to have a space between methods. That's not required by rustfmt, but is the more typical way things are formatted.
| pub fn add(&mut self, delegate: &T) -> Result<i64> { | ||
| let mut _lock_free_drop = Array::new(); | ||
| Ok({ | ||
| let change_lock = self.change.lock().unwrap(); |
There was a problem hiding this comment.
I'm surprised you don't get a warning here for an unused variable. I think this should be _change_lock. Similiarly, I think _lock_free_drop shouldn't have the _ prefix since this convention is used for unused bindings not bindings that are of secondary importance like here.
There was a problem hiding this comment.
I think _lock_free_drop shouldn't have the _ prefix
If I remove it, compiler complains with value assigned to lock_free_drop is never read.
| 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() { | ||
| new_delegates.push(delegate.clone()); |
There was a problem hiding this comment.
Why do the delegates have to be cloned instead of the new delegate just being pushed on to the existing delegates array?
There was a problem hiding this comment.
To avoid disrupting a concurrent read of the delegates, a new array needs to be created every time. The tension is between a thread adding/removing a delegate and another thread firing the event and thus cycling through the existing array. The vast majority of event sources have either zero or one handler, and this was determined to be the most efficient implementation.
| let mut new_delegates = Array::new(); | ||
| let mut removed = false; | ||
| if capacity == 0 { | ||
| if self.delegates.as_slice()[0].to_token() == token { |
There was a problem hiding this comment.
nit: we might want to just add indexing directly to the Array type
| let mut new_delegates = Array::new(); | ||
| let mut removed = false; | ||
| if capacity == 0 { | ||
| if self.delegates.as_slice()[0].to_token() == token { |
There was a problem hiding this comment.
removed = self.delegates.as_slice()[0].to_token() == token;| Ok(std::ptr::null_mut()) | ||
| } else { | ||
| let alloc_size = std::mem::size_of::<Buffer>() + size; | ||
| let header = heap_alloc(alloc_size)? as *mut Buffer; |
There was a problem hiding this comment.
The other code makes assumptions that this pointer is properly aligned. Since Buffer is an AtomicI32 can we assume that the pointer will always be properly aligned?
There was a problem hiding this comment.
Yes, HeapAlloc returns memory is aligned to 8 bytes on x86 and 16 bytes on x64.
| /// Appends a delegate to the back of the array. | ||
| fn push(&mut self, delegate: Delegate<T>) { | ||
| unsafe { | ||
| std::ptr::write((*self.buffer).as_mut_ptr::<Delegate<T>>().add(self.len) as _, delegate); |
There was a problem hiding this comment.
What happens if we push beyond the capacity of the buffer?
| if !self.is_empty() { | ||
| unsafe { (*self.buffer).0.add_ref() }; | ||
| } | ||
| Self { buffer: self.buffer, len: self.len, _phantom: std::marker::PhantomData } |
There was a problem hiding this comment.
If I clone this Array I then have two Array<T> which I can call push on at the same time. However, if they are pointing to the same buffer, I might be overwriting their contents. This would seem to violate memory safety, no?
There was a problem hiding this comment.
This is just bumping the ref count on the same array. Basically, there are now two smart pointers to the same array to support lock-free calling. The swap/change locks ensures we don't touch the array being used for calls.
| } | ||
|
|
||
| /// A thread-safe reference-counted array of delegates. | ||
| struct Array<T: Interface + Clone> { |
There was a problem hiding this comment.
Nit: typically, the bounds on a generic param are only given where they're needed. So, here we would not constrain T but we would keep the constraint where it's really needed (on the inherit impl and the Drop impl).
There was a problem hiding this comment.
I tried that but Rust complains:
Checking windows v0.37.0 (D:\git\windows-rs\crates\libs\windows)
error[E0367]: `Drop` impl requires `T: interface::Interface` but the struct it is implemented for does not
--> crates\libs\windows\src\core\event.rs:190:9
|
190 | impl<T: Interface + Clone> Drop for Array<T> {
| ^^^^^^^^^
|
note: the implementor must specify the same requirement
--> crates\libs\windows\src\core\event.rs:114:1
|
114 | / struct Array<T> {
115 | | buffer: *mut Buffer,
116 | | len: usize,
117 | | _phantom: std::marker::PhantomData<T>,
118 | | }
| |_^
error[E0367]: `Drop` impl requires `T: Clone` but the struct it is implemented for does not
--> crates\libs\windows\src\core\event.rs:190:21
|
190 | impl<T: Interface + Clone> Drop for Array<T> {
| ^^^^^
|
note: the implementor must specify the same requirement
--> crates\libs\windows\src\core\event.rs:114:1
|
114 | / struct Array<T> {
115 | | buffer: *mut Buffer,
116 | | len: usize,
117 | | _phantom: std::marker::PhantomData<T>,
118 | | }
| |_^
For more information about this error, try `rustc --explain E0367`.
error: could not compile `windows` due to 2 previous errors
| /// Creates a new `Delegate<T>`, containing a suitable reference to the specified delegate. | ||
| fn new(delegate: &T) -> Self { | ||
| if delegate.cast::<IAgileObject>().is_err() { | ||
| if let Ok(delegate) = AgileReference::new(delegate) { |
There was a problem hiding this comment.
If delegate does not implement IAgileObject but we can't create an AgileReference shouldn't we error?
There was a problem hiding this comment.
This is a back door that C++ uses to handle Xaml's internal delegates which refuse to support agility or marshaling, but I agree it's a hack and Rust doesn't need to support it.
The implementation is based on the event class in cppwinrt. The notable difference is that Rust lacks something akin to C++ variadic templates so we can't just create an agile delegate wrapper and must instead use an explicit enum. Beyond that, the implementation is largely the same to simplify review and validation. The Rust
Event<T>implementation is thus thread-safe and largely equivalent to C++/WinRT and WRL in terms of behavior and performance.A part of #1094.