Skip to content

[chaininterface] Add ability for BlockNotifier to unregister listeners #502

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

Merged
Merged
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
87 changes: 86 additions & 1 deletion lightning/src/chain/chaininterface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use std::sync::atomic::{AtomicUsize, Ordering};
use std::collections::HashSet;
use std::ops::Deref;
use std::marker::PhantomData;
use std::ptr;

/// Used to give chain error details upstream
pub enum ChainError {
Expand Down Expand Up @@ -252,11 +253,22 @@ impl<'a, CL: Deref<Target = ChainListener + 'a> + 'a> BlockNotifier<'a, CL> {
}

/// Register the given listener to receive events.
// TODO: unregister
pub fn register_listener(&self, listener: CL) {
let mut vec = self.listeners.lock().unwrap();
vec.push(listener);
}
/// Unregister the given listener to no longer
/// receive events.
///
/// If the same listener is registered multiple times, unregistering
/// will remove ALL occurrences of that listener. Comparison is done using
/// the pointer returned by the Deref trait implementation.
pub fn unregister_listener(&self, listener: CL) {
let mut vec = self.listeners.lock().unwrap();
// item is a ref to an abstract thing that dereferences to a ChainListener,
// so dereference it twice to get the ChainListener itself
vec.retain(|item | !ptr::eq(&(**item), &(*listener)));
}

/// Notify listeners that a block was connected given a full, unfiltered block.
///
Expand Down Expand Up @@ -376,3 +388,76 @@ impl ChainWatchInterfaceUtil {
watched.does_match_tx(tx)
}
}

#[cfg(test)]
mod tests {
use ln::functional_test_utils::{create_node_cfgs};
use super::{BlockNotifier, ChainListener};
use std::ptr;

#[test]
fn register_listener_test() {
let node_cfgs = create_node_cfgs(1);
let block_notifier = BlockNotifier::new(node_cfgs[0].chain_monitor.clone());
assert_eq!(block_notifier.listeners.lock().unwrap().len(), 0);
let listener = &node_cfgs[0].chan_monitor.simple_monitor as &ChainListener;
block_notifier.register_listener(listener);
let vec = block_notifier.listeners.lock().unwrap();
assert_eq!(vec.len(), 1);
let item = vec.first().clone().unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a test that you can deregister node_cfgs[0].chan_monitor.simple_monitor itself instead of just the thing that was pulled out of the registration list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I fully understand what you mean by

instead of just the thing that was pulled out of the registration list

Do you mean calling unregister_listener and passing in node_cfgs[0].chan_monitor.simple_monitor itself instead of the listener1 local variable?

If so, I wrote a new test unregister_single_listener_ref_test, otherwise would love some guidance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thats exactly what I meant.

assert!(ptr::eq(&(**item), &(*listener)));
}

#[test]
fn unregister_single_listener_test() {
let node_cfgs = create_node_cfgs(2);
let block_notifier = BlockNotifier::new(node_cfgs[0].chain_monitor.clone());
let listener1 = &node_cfgs[0].chan_monitor.simple_monitor as &ChainListener;
let listener2 = &node_cfgs[1].chan_monitor.simple_monitor as &ChainListener;
block_notifier.register_listener(listener1);
block_notifier.register_listener(listener2);
let vec = block_notifier.listeners.lock().unwrap();
assert_eq!(vec.len(), 2);
drop(vec);
block_notifier.unregister_listener(listener1);
let vec = block_notifier.listeners.lock().unwrap();
assert_eq!(vec.len(), 1);
let item = vec.first().clone().unwrap();
assert!(ptr::eq(&(**item), &(*listener2)));
}

#[test]
fn unregister_single_listener_ref_test() {
let node_cfgs = create_node_cfgs(2);
let block_notifier = BlockNotifier::new(node_cfgs[0].chain_monitor.clone());
block_notifier.register_listener(&node_cfgs[0].chan_monitor.simple_monitor as &ChainListener);
block_notifier.register_listener(&node_cfgs[1].chan_monitor.simple_monitor as &ChainListener);
let vec = block_notifier.listeners.lock().unwrap();
assert_eq!(vec.len(), 2);
drop(vec);
block_notifier.unregister_listener(&node_cfgs[0].chan_monitor.simple_monitor);
let vec = block_notifier.listeners.lock().unwrap();
assert_eq!(vec.len(), 1);
let item = vec.first().clone().unwrap();
assert!(ptr::eq(&(**item), &(*&node_cfgs[1].chan_monitor.simple_monitor)));
}

#[test]
fn unregister_multiple_of_the_same_listeners_test() {
let node_cfgs = create_node_cfgs(2);
let block_notifier = BlockNotifier::new(node_cfgs[0].chain_monitor.clone());
let listener1 = &node_cfgs[0].chan_monitor.simple_monitor as &ChainListener;
let listener2 = &node_cfgs[1].chan_monitor.simple_monitor as &ChainListener;
block_notifier.register_listener(listener1);
block_notifier.register_listener(listener1);
block_notifier.register_listener(listener2);
let vec = block_notifier.listeners.lock().unwrap();
assert_eq!(vec.len(), 3);
drop(vec);
block_notifier.unregister_listener(listener1);
let vec = block_notifier.listeners.lock().unwrap();
assert_eq!(vec.len(), 1);
let item = vec.first().clone().unwrap();
assert!(ptr::eq(&(**item), &(*listener2)));
}
}
3 changes: 2 additions & 1 deletion lightning/src/ln/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ mod onion_utils;
mod wire;

#[cfg(test)]
#[macro_use] mod functional_test_utils;
#[macro_use]
pub(crate) mod functional_test_utils;
#[cfg(test)]
mod functional_tests;
#[cfg(test)]
Expand Down