-
Notifications
You must be signed in to change notification settings - Fork 918
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
Create ListenersEvent::Closed on Swarm::remove_listener #2261
Conversation
74eaef5
to
0efe82c
Compare
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.
I am in favor of this change.
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.
Thank you! One question inline.
core/src/connection/listeners.rs
Outdated
let listener_project = listener.as_mut().project(); | ||
self.pending_events.push_back(ListenersEvent::Closed { | ||
listener_id: *listener_project.id, | ||
addresses: listener_project.addresses.drain(..).collect(), |
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.
Would std::mem::replace
also work here? Slightly more readable IMO.
Giving others a chance to comment on the latest changes. Will merge early next week in case there are no further comments. |
Thanks @elenaf9! |
Discussion #2113.
Create a
ListenersEvent::Closed
when a listener is removed viaSwarm::remove_listener
.This makes it more consistent with
Swarm::listen_on
, and also informs the Swarm about the associated expired addresses.I initially tried to realize it via a
Stream
implementation on theconnection::Listener
, so that it would just returnPoll::Ready(None)
if the listener was removed (see this commit). But due to Listener being wrapped in aPin
it is a bit more complicated, whereas this Draft now is only a very minimal change.Note: 0efe82c introduces a minor API change so that
remove_listener
returns a boolean instead ofResult<(), ()>
. This 1. more consistent with other methods likeremove_external_address
, and 2. follows the clippy lint of result_unit_err.