-
Notifications
You must be signed in to change notification settings - Fork 33
Add iterator and accessor to EventManager #69
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
// Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
// SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause | ||
|
||
use std::collections::hash_map::{Iter, IterMut}; | ||
use std::mem::size_of; | ||
|
||
use vmm_sys_util::epoll::EpollEvent; | ||
|
@@ -56,6 +57,11 @@ impl<T: MutEventSubscriber> SubscriberOps for EventManager<T> { | |
Ok(subscriber) | ||
} | ||
|
||
/// Return a reference to the subscriber associated with the provided id. | ||
fn subscriber_ref(&self, subscriber_id: SubscriberId) -> Option<&T> { | ||
self.subscribers.get_subscriber(subscriber_id) | ||
} | ||
|
||
/// Return a mutable reference to the subscriber associated with the provided id. | ||
fn subscriber_mut(&mut self, subscriber_id: SubscriberId) -> Result<&mut T> { | ||
if self.subscribers.contains(subscriber_id) { | ||
|
@@ -181,6 +187,16 @@ impl<S: MutEventSubscriber> EventManager<S> { | |
#[cfg(feature = "remote_endpoint")] | ||
self.dispatch_endpoint_event(endpoint_event); | ||
} | ||
|
||
/// An iterator visiting all subscribers in arbitrary order, with immutable references. | ||
pub fn iter(&mut self) -> Iter<'_, SubscriberId, S> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this use an immutable reference |
||
self.subscribers.iter() | ||
} | ||
|
||
/// An iterator visiting all subscribers in arbitrary order, with mutable references. | ||
pub fn iter_mut(&mut self) -> IterMut<'_, SubscriberId, S> { | ||
self.subscribers.iter_mut() | ||
} | ||
} | ||
|
||
#[cfg(feature = "remote_endpoint")] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ | |
// SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause | ||
|
||
use super::SubscriberId; | ||
use std::collections::HashMap; | ||
use std::collections::hash_map::{HashMap, Iter, IterMut}; | ||
|
||
// Internal structure used to keep the set of subscribers registered with an EventManger. | ||
// This structure is a thin wrapper over a `HashMap` in which the keys are uniquely | ||
|
@@ -51,4 +51,19 @@ impl<T> Subscribers<T> { | |
pub(crate) fn get_mut_unchecked(&mut self, subscriber_id: SubscriberId) -> &mut T { | ||
self.subscribers.get_mut(&subscriber_id).unwrap() | ||
} | ||
|
||
// Return a reference to the subriber represented by `subscriber_id`. | ||
pub(crate) fn get_subscriber(&self, subscriber_id: SubscriberId) -> Option<&T> { | ||
self.subscribers.get(&subscriber_id) | ||
} | ||
|
||
/// An iterator visiting all subscribers in arbitrary order, with immutable references. | ||
pub(crate) fn iter(&mut self) -> Iter<'_, SubscriberId, T> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this use an immutable reference |
||
self.subscribers.iter() | ||
} | ||
|
||
/// An iterator visiting all subscribers in arbitrary order, with mutable references. | ||
pub(crate) fn iter_mut(&mut self) -> IterMut<'_, SubscriberId, T> { | ||
self.subscribers.iter_mut() | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,15 @@ impl App { | |
let _ = self.event_manager.run_with_timeout(100); | ||
} | ||
|
||
fn iter(&mut self) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check doesn't really fit here. This is the example that we're also referencing in the public documentation, so I'd like to keep it as close as we can to a real use life use case. How about we move this to a separate test instead of having it part of the |
||
for (k, _) in self.event_manager.iter_mut() { | ||
assert!(self.subscribers_id.contains(k)); | ||
} | ||
for (k, _) in self.event_manager.iter() { | ||
assert!(self.subscribers_id.contains(k)); | ||
} | ||
} | ||
|
||
fn inject_events_for(&mut self, subscriber_index: &[usize]) { | ||
for i in subscriber_index { | ||
let subscriber = self | ||
|
@@ -58,7 +67,7 @@ impl App { | |
fn get_counters(&mut self) -> Vec<u64> { | ||
let mut result = Vec::<u64>::new(); | ||
for subscriber_id in &self.subscribers_id { | ||
let subscriber = self.event_manager.subscriber_mut(*subscriber_id).unwrap(); | ||
let subscriber = self.event_manager.subscriber_ref(*subscriber_id).unwrap(); | ||
result.push(subscriber.counter()); | ||
} | ||
|
||
|
@@ -71,6 +80,7 @@ impl App { | |
for id in &self.subscribers_id { | ||
let _ = self.event_manager.remove_subscriber(*id); | ||
} | ||
self.subscribers_id.clear(); | ||
} | ||
} | ||
|
||
|
@@ -91,6 +101,7 @@ fn test_single_threaded() { | |
let triggered_subscribers: Vec<usize> = vec![1, 3, 50, 97]; | ||
app.inject_events_for(&triggered_subscribers); | ||
app.run(); | ||
app.iter(); | ||
|
||
let counters = app.get_counters(); | ||
for i in 0..100 { | ||
|
@@ -104,8 +115,13 @@ fn test_single_threaded() { | |
assert_eq!(counters[i], 1 & (triggered_subscribers.contains(&i) as u64)); | ||
} | ||
|
||
let id = app.subscribers_id[0]; | ||
|
||
// Once the app does not need events anymore, the cleanup needs to be called. | ||
// This is particularly important when the app continues the execution, but event monitoring | ||
// is not necessary. | ||
app.cleanup(); | ||
|
||
assert!(app.event_manager.subscriber_ref(id).is_none()); | ||
assert!(app.event_manager.subscriber_mut(id).is_err()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this check the error more explicitly? E.g. |
||
} |
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.
Can we return a
Result
here as well so that it in line with the other getter we have for subscribers (i.e. thesubscriber_mut
)?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.
Can you also add a test for when the subscriber was removed, and then we call
subscriber_ref
?