add unregister_listener functionality to ChainListener

[rloomba]

Per comment in chaininterface.rs, add the ability to unregister a chain listener.

Happy to take this on as a good first issue.

[ariard]

Go for it :)

[rloomba]

One idea is to try and utilize std::ptr::eq to compare pointer equality, in order to figure out the correct vector index to use for removal. Something like this:

pub fn remove_listener(&self, listener: CL) {
    let mut vec = self.listeners.lock().unwrap();
    for item in vec.iter() {
//      if(std::ptr::eq(&item, &listener)){
//         println!("use index of this item for removal");
//      }
}

However, I'm not certain this works for Trait objects. And this doesn't feel like the "Rust" way of doing things. Even it this approach did work, it wouldn't support removing a single listener that was added twice, e.g.:

block_notifier.register_listener(&simple_monitor);
block_notifier.register_listener(&simple_monitor);
block_notifier.unregister_listener(&simple_monitor); // would remove both listeners

Another approach could be to implement PartialEq for all structs that implement the ChainListener trait(SimpleManyChannelMonitor and ChannelManager).

pub fn unregister_listener(&self, listener: CL) -> bool {
    let mut vec = self.listeners.lock().unwrap();
    if let Some((idx, _)) = vec.iter().enumerate().find(|&(_, cl)| *cl == listener) {
        vec.remove(idx);
        true
    } else {
        false
    }
}

and some clever logic to convert to Any

impl ChainListener for ChannelManager {
    fn eq(&self, other: &ChainListener) -> bool {
        other
            .as_any()
            .downcast_ref::<Self>()
            .map_or(false, |x| x == self)
    }

    fn as_any(&self) -> &Any {
        self
    }
}

#[derive(PartialEq)] on SimpleManyChannelMonitor or ChannelManager seems to indicate that PartialEq would need to be implemented for many members of the struct -- this seems like a pretty big refactor.... Also this approach might run into issues when two "instances" of the same listener is added.

Another approach could be to generate a uid each time a listener is added, and then return this uid.

let listener_id = block_notifier.register_listener(&simple_monitor);

Now, when we want to remove a listener, the uid can be used for the lookup.

@ariard any thoughts?

[ariard]

Honestly I'm fine with the uid approach, it seems easier for a client to store uid of all its LN components than having to deal with pointers. Uids may be even better for passing them through different memory spaces.

@valentinewallace you worked a lot on this recently ?

[TheBlueMatt]

std::ptr::eq seems fine to me - the pointer is essentially to the vtable + self parameter, which defines the full set of things the user will see at event-time, so registering copies would be pretty useless, and deregistering based on that seems to encompass everything the user would want. Of this would require the documentation be very clear (that no attempt is made to avoid duplicate registration, and deregister removes them all, or is undefined, up to you).

…

On Feb 9, 2020, at 17:57, Antoine Riard ***@***.***> wrote:  Honestly I'm fine with the uid approach, it seems easier for a client to store uid of all its LN components than having to deal with pointers. Uids may be even better for passing them through different memory spaces. @valentinewallace you worked a lot on this recently ? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[valentinewallace]

I find the std::ptr::eq approach more intuitive (not super strongly though, if uid is preferred).

It should be possible to deregister without removing duplicates, though? Just find the index of the first one and only remove that? (Which also isn't a big deal, though as Matt says, clear docs would be good here.)

[rloomba]

I dug into the std::ptr::eq approach more, and I think since we're using generics, our trait object fat pointers that contain our vtable + data will not be equal.

Here's a snippet that demonstrates this:

//#![feature(raw)]
use std::sync::Mutex;
use std::ops::Deref;
use std::sync::Arc;
use std::mem::transmute;
use std::fmt::Debug;

//use std::{mem, raw};

pub trait Listener: Sync + Send {
    fn listen_for_new_block(&self);
}

pub struct Node {}

impl Node {
    pub fn new() -> Self {
        Self {}
    }
}
pub struct ChannelManager {}

impl ChannelManager {
    pub fn new() -> Self {
        Self {}
    }
}

impl Listener for Node {
    fn listen_for_new_block(&self) {
        println!("node listening for new block")
    }
}

impl Listener for ChannelManager {
    fn listen_for_new_block(&self) {
        println!("channel manager listening for new block")
    }
}

pub type DerefBlockArc = Arc<DerefBlockNotifier<'static, Arc<Listener>>>;

pub type DerefBlockRef<'a> = DerefBlockNotifier<'a, &'a Listener>;

pub struct DerefBlockNotifier<'a, L: Deref<Target = Listener + 'a> + 'a> {
    items: Mutex<Vec<L>>
}

impl<'a, L: Deref<Target = Listener + 'a> + 'a> DerefBlockNotifier<'a, L> {
    pub fn new() -> DerefBlockNotifier<'a, L> {
        DerefBlockNotifier {
            items: Mutex::new(Vec::new())
        }
    }

    pub fn add(&self, listener: L) {
        let mut vec = self.items.lock().unwrap();
        vec.push(listener);
    }

    pub fn remove(&self, listener: L) {
        let mut vec = self.items.lock().unwrap();
        for item in vec.iter() {

            // check size, these are 16 bytes, which implies a fat pointer
            let size_item = std::mem::size_of_val(item);
            let alignment_item = std::mem::align_of_val(item);

            let size_listener = std::mem::size_of_val(&listener);
            let alignment_listener = std::mem::align_of_val(&listener);

            // unstable raw:TraitObject splits raw pointer into data/vtable
//            let raw_listener = &listener as *const _ as *mut std::raw::TraitObject;
//            let raw_item = item as *const _ as *mut std::raw::TraitObject;

            let raw_listener = &listener as *const _ as *const (*const (), *const());
            let raw_item = item as *const _ as *const (*const (), *const());


            // raw pointers are NOT equal
            assert!(!std::ptr::eq(item as *const _ as *const (), &listener as *const _ as *const ()));

            // data pointers ARE equal
            let data_pointer_equality = std::ptr::eq(unsafe { (*raw_item).0 }, unsafe { (*raw_listener).0 });
            assert!(data_pointer_equality);

            // vtable pointers ARE equal
            let vtable_pointer_equality = std::ptr::eq(unsafe { (*raw_item).1 }, unsafe { (*raw_listener).1 });
            assert!(vtable_pointer_equality);


            if(data_pointer_equality && vtable_pointer_equality){
                println!("listeners are equal, should remove this one");
            }

        }

    }

}

fn main() {
    let node= Node::new();
    let ch_mgr = ChannelManager::new();

    let notifier: DerefBlockNotifier<&dyn Listener> = DerefBlockNotifier::new();
    notifier.add(&node as &Listener);
    notifier.remove(&node as &Listener);
}

Link to Rust Playground with above code

The data + vtable pointers are equal, but the fat pointer struct that encapsulates them is not. The above approach is not only unsafe, but also assumes the structure of the fat pointer, which might change in the future.

Here's an RFC that's tracking changes to exposing pointer metadata + vtable.

And it looks like the raw::TraitObject is not coming to stable anytime soon.

I'm leaning towards the uid approach. We will require the user to supply randomness to generate the uid. So the flow might look like:

 let random_bytes = rand::thread_rng().gen::<[u8; 32]>();
let uid = Uid::new(random_byes);
block_notifier.register_listener(&node as &ChainListener, uid);
block_notifier.unregister_listener(uid);

thoughts?

[TheBlueMatt]

Hmm, adding this line to your fn works on playground, though:

          assert!(std::ptr::eq(&(**item) as *const _, &(*listener) as *const _));

which seems to be what we want - deref the object to get the Listener itself, then compare.

[rloomba]

lol wow. 🤦‍♂. Thanks @TheBlueMatt . Will work on a patch using this approach and document appropriately.