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

Conversation

rloomba
Copy link
Contributor

@rloomba rloomba commented Feb 15, 2020

This resolves #473.

Note: If a user has registered the same listener multiple times, unregister_listener will remove all occurrences of that listener.

@rloomba rloomba force-pushed the rloomba/add_unregister_listener branch from 5b7690a to 9bfcac6 Compare February 15, 2020 19:26
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Looks good, all rather trivial nits.

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.

@TheBlueMatt TheBlueMatt added this to the 0.0.10 milestone Feb 17, 2020
…ered listener, in order to no longer receive events
@rloomba rloomba force-pushed the rloomba/add_unregister_listener branch from 9bfcac6 to 7d62346 Compare February 18, 2020 05:34
@TheBlueMatt TheBlueMatt merged commit 2f34641 into lightningdevkit:master Feb 19, 2020
@TheBlueMatt TheBlueMatt modified the milestones: 0.0.10, 0.0.11 Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add unregister_listener functionality to ChainListener
2 participants