Skip to content
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

Add iterator and accessor to EventManager #69

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion coverage_config_x86_64.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"coverage_score": 85.6,
"coverage_score": 87.0,
"exclude_path": "tests/,benches/,utilities/",
"crate_features": "remote_endpoint,test_utilities"
}
22 changes: 22 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@ pub trait SubscriberOps {
/// Removes the subscriber corresponding to `subscriber_id` from the watch list.
fn remove_subscriber(&mut self, subscriber_id: SubscriberId) -> Result<Self::Subscriber>;

/// Returns a reference to the subscriber corresponding to `subscriber_id`.
fn subscriber_ref(&self, subscriber_id: SubscriberId) -> Option<&Self::Subscriber>;
Copy link
Member

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. the subscriber_mut)?

Copy link
Member

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?


/// Returns a mutable reference to the subscriber corresponding to `subscriber_id`.
fn subscriber_mut(&mut self, subscriber_id: SubscriberId) -> Result<&mut Self::Subscriber>;

Expand Down Expand Up @@ -276,3 +279,22 @@ impl<T: MutEventSubscriber + ?Sized> MutEventSubscriber for Box<T> {
self.deref_mut().init(ops);
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_subscriber_id_derives() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason for checking the derives? We considered this to be coverage tests, and we avoided them in the past. I am wondering if there is something particular about the SubscriberId that we want to validate, or we could remove them.

let a = SubscriberId(1);
let b = SubscriberId(1);
let c = SubscriberId(2);

assert_eq!(a, b);
assert_ne!(a, c);
assert_ne!(c, b);

let d = c.clone();
assert_eq!(c, d);
}
}
43 changes: 43 additions & 0 deletions src/manager.rs
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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this use an immutable reference &self? (duplicate of 49ba509#r1168308920)

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")]
Expand Down Expand Up @@ -481,4 +497,31 @@ mod tests {
Error::InvalidId
);
}

#[test]
#[cfg(feature = "remote_endpoint")]
fn test_endpoint() {
use std::thread;

let mut event_manager = EventManager::<DummySubscriber>::new().unwrap();
let dummy = DummySubscriber::new();
let endpoint = event_manager.remote_endpoint();
let kicker = event_manager.remote_endpoint();

let thread_handle = thread::spawn(move || {
event_manager.run().unwrap();
event_manager.run().unwrap();
});

dummy.event_fd_1.write(1).unwrap();

let token = endpoint
.call_blocking(|sub_ops| -> Result<SubscriberId> { Ok(sub_ops.add_subscriber(dummy)) })
.unwrap();
assert_eq!(token, SubscriberId(1));

kicker.kick().unwrap();

thread_handle.join().unwrap();
}
}
17 changes: 16 additions & 1 deletion src/subscribers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this use an immutable reference &self? (duplicate of 49ba509#r1168309051)

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()
}
}
18 changes: 17 additions & 1 deletion tests/basic_event_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,15 @@ impl App {
let _ = self.event_manager.run_with_timeout(100);
}

fn iter(&mut self) {
Copy link
Member

Choose a reason for hiding this comment

The 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 App interface? We can add a test_iter and move the following asserts there after initializing the subscribers.

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
Expand All @@ -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());
}

Expand All @@ -71,6 +80,7 @@ impl App {
for id in &self.subscribers_id {
let _ = self.event_manager.remove_subscriber(*id);
}
self.subscribers_id.clear();
}
}

Expand All @@ -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 {
Expand All @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this check the error more explicitly?

E.g. assert_eq!(app.event_manager.subscriber_mut(id), Err(..));

}