-
Notifications
You must be signed in to change notification settings - Fork 507
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Immutable Event implementation #3198
Conversation
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
@microsoft-github-policy-service agree |
Additional synchronization should not be required above the swap and change locks. For reference, I wrote the Rust implementation based on the implementation I wrote for C++/WinRT here: https://github.com/microsoft/cppwinrt/blob/master/strings/base_events.h That in turn is loosely based on the WRL implementation. |
Basically all we need to do here is make |
Assuming we have an exclusive access to the array
Assuming we have an exclusive access to the array
Assuming we have exclusive access to the array
Since we have exclusive access to the delegate array when we hold the |
Looks good - is it necessary to use a pointer rather than a mutable reference? |
use core::cell::UnsafeCell.
I figured out a way to use a mutable reference using UnsafeCell. Once the class design is complete, I guess we should impl Send and Sync right 🤔 |
Yes, it can be |
Would appreciate some more eyes on this. The change here is just to make the |
Seems reasonable to me. @sivadeilra thoughts? |
Reading. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this code is safe (sound). It accesses the delegates
list while acquiring different locks in different pub
methods.
Can you explain why this is safe? If delegates
is associated with exactly one lock, then why is it not contained within one of the Mutex
instances? Since this code acquires different locks in different paths, I don't understand how it is sound.
From my understanding:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I re-read this with Chris Denton's comments in mind.
I hate to say it, but I think this code makes the wrong trade-off of safety vs. performance. First, this code may currently be sound, but it is dangerously close to the edge. And there are safe abstractions that already provide the semantics that are wanted.
From my reading of this code, RwLock
would provide exactly the same level of synchronization, and would do so safely. The only scenario where RwLock
would have different behavior would be that writers would briefly block reads (call
paths), but I consider that acceptable for this design because modifying the list should be extremely infrequent, compared to reading the list.
So I think this PR needlessly uses unsafe
code; there is no advantage here to using unsafe
code. Even worse, this PR mixes the implementation of a R/W lock with a particular usage of that R/W lock, so it mixes two things together. That makes verification harder and makes it more likely that a future PR that looks innocent will actually break the safety invariants. And there's just no advantage over using RwLock
.
I'm also troubled by the performance implication of cloning the delegate list every time call
runs. That's a lot of allocator calls.
I think this would be improved by replacing all of this with RwLock<Arc<Array<T>>>
. The writer paths (add
, remove
, etc.) would acquire it in exclusive mode and do the obvious thing. The call
path would acquire the RwLock
in shared mode, call Arc::clone
(which is a single interlocked increment), then drop the RwLock
guard. This would eliminate all of the allocator calls in call
.
This implementation would be 100% safe code, and obviously correct from a borrow-checker and concurrency point of view.
Thanks for the feedback. As I mentioned in #3198 (comment), this implementation is copied from C++/WinRT which is based on WRL. The choice around performance tradeoffs was done before my time but it was stressed that C++/WinRT must provide the same performance tradeoffs as WRL as they were calculated at some point to be preferable for the OS. Having said that, I don't mind making a different set of performance tradeoffs for Rust if it means the code is simpler and thus easier to reason about. I will say that this implementation (at least the C++ implementations) has stood the test of time and very heavy usage. However, there are some constraints that must be preserved. I suspect
If a simpler implementation is possible that preserves these constraints, then I'm not opposed to it. 😊 |
Both of those constraints would be met by using I think a simpler, provably-safe implementation is the best place to start. If there is quantitative evidence that this implementation can't meet the same needs as the equivalent C++, then I'm open to adjusting that, but not without some evidence. If it would help, I can sketch out the implementation. |
Hey @lifers I realize this is probably more than you bargained for. 😊 Would you like someone else to handle this? |
Hi, this has gotten more interesting and I'd like to figure this out unless the maintainers need to finish this ASAP with their background knowledge 😄. It takes a while for me to figure out what else is happening inside the initial code. From what I've seen,
|
A few thoughts.
I wonder if we can use |
So here's a sketch of what I propose:
Obviously this is just a sketch, but the idea should be clear. The paths that modify the list use |
If the fallible allocation is still needed, we could use a simplified /// A thread-safe reference-counted array of delegates.
struct Array<T: Interface> {
buffer: *mut Delegate<T>,
len: usize,
}
impl<T: Interface> Default for Array<T> {
fn default() -> Self {
Self::with_capacity(0).unwrap()
}
}
impl<T: Interface> Array<T> {
/// Creates a new, empty `Array<T>` with the specified capacity.
fn with_capacity(capacity: usize) -> Result<Self> {
if capacity != 0 {
let alloc_size = capacity * size_of::<Delegate<T>>();
let header = heap_alloc(alloc_size)? as *mut Delegate<T>;
Ok(Self {
buffer: header,
len: 0,
})
} else {
Ok(Self {
buffer: null_mut(),
len: 0,
})
}
} which will be used as pub struct Event<T: Interface> {
delegates: RwLock<Arc<Array<T>>>,
}
impl<T: Interface> Event<T> {
pub fn add(&self, delegate: T) -> Result<()> {
let mut guard = self.delegates.write().unwrap();
let mut new_list = Array::with_capacity(guard.len() + 1)?;
for old_delegate in guard.as_slice().iter() {
new_list.push(old_delegate.clone());
}
new_list.push(Delegate::new(&delegate)?);
// let new_list_arc: Arc<[Delegate<T>]> = new_list.into(); // <-- converts Vec<T> into Arc<[T]>
*guard = new_list.into();
Ok(())
}
pub fn clear(&self) -> Result<()> {
let mut guard = self.delegates.write().unwrap();
*guard = Array::with_capacity(0)?.into();
Ok(())
}
...
pub fn call(&self) {
let delegates = {
let guard = self.delegates.read().unwrap();
guard.clone()
// <-- lock is released here
};
for delegate in delegates.as_slice().iter() {
// ...
}
} |
Don't worry about allocation failure. |
Honestly, I think providing the "fallible" allocation pathway is worse. Just stick with the standard Rust approach. If you are implementing a memory allocation container, the appropriate response to OOM is to call |
The `Option` that wraps `Arc<[Delegate<T>]>` will let us assign `None` instead of creating `Arc::new([])` with no space penalty.
Looks a lot simpler -- thank you! The Clippy suggestion about a |
Now we only need to allocate once for Arc
By the way, now that |
Concurrency tests can be challenging - fortunately the code is now a lot simpler so perhaps just a basic smoke test confirming that it is in fact |
Just an idea for a new test...```Rust #[test] fn across_threads() -> Result<()> { let event = Arc::new(Event::>::new());
}
|
I'm not opposed to that test, but from experience in testing concurrent algorithms, I don't think it actually has much value. I know, more testing is always supposed to be better, but when it comes to concurrency, tests can often be flakey, or even if they aren't flakey, they don't actually exercise meaningful properties and so they just give a false sense of security. If you think this test is valuable, and it runs in a very short period of time, I'm fine with it. However, don't drop a thread joiner; that's nearly always a code smell. If you think you've already synchronized with the thread, then retain the thread joiner and join on it at the end of the test function. It should be a no-op, but it gives me higher confidence in the correctness of the test. |
Yep, I'd not worry about testing the concurrency itself and just test that the compiler can actually move the references around - the thing that wouldn't work if |
Should fail to compile if not
Co-authored-by: Kenny Kerr <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate all the work on this - looks great!
Glad to help! 😁 Thanks everyone for following through. |
Implement the Event struct with no need of mutating methods using RwLock. I'd think it's safe from deadlock because:
Please let me know if there is other considerations for this feature. Thank you!
Fixes: #1833