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

Pass the error to inject_listener_closed method #1517

Merged
merged 5 commits into from
Apr 1, 2020

Conversation

tcharding
Copy link
Contributor

If there is an error when the listener closes, found in the NetworkEvent::ListenerClosed reason field, we would like to pass it on to the inject_listener_closed() method so that implementors of this method have access to it.

Add an error parameter to inject_listener_closed. Convert the reason field from a Result to an Option and if there is an error pass Some(error) at the method call site.

Resolves: #1475

If there is an error when the listener closes, found in the
`NetworkEvent::ListenerClosed` `reason` field, we would like to pass it
on to the `inject_listener_closed()` method so that implementors of this
method have access to it.

Add an error parameter to `inject_listener_closed`.  Convert the
`reason` field from a `Result` to an `Option` and if there is an error
pass `Some(error)` at the method call site.
@@ -130,7 +130,7 @@ pub trait NetworkBehaviour: Send + 'static {
}

/// A listener closed.
fn inject_listener_closed(&mut self, _id: ListenerId) {
fn inject_listener_closed(&mut self, _id: ListenerId, _err: Option<std::io::Error>) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you leave the Result for consistency with the rest of the library?

Suggested change
fn inject_listener_closed(&mut self, _id: ListenerId, _err: Option<std::io::Error>) {
fn inject_listener_closed(&mut self, _id: ListenerId, reason: Result<(), io::Error>) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, can do.

@tcharding
Copy link
Contributor Author

After the merge conflicts are fixed we get left with this (buggy) code:

for addr in addresses.iter() {
    this.behaviour.inject_expired_listen_addr(addr);
}
this.behaviour.inject_listener_closed(listener_id, reason);
return Poll::Ready(SwarmEvent::ListenerClosed {
    addresses,
    reason,
});

The compiler error is that reason (due to the error being io::Error) does not implement Copy or Clone. Seems like we have to make a choice, either pass it to the trait method (inject_listener_closed) or pass it up in the event - we cannot do both. Any thoughts please @tomaka?

@tomaka
Copy link
Member

tomaka commented Apr 1, 2020

Please pass the error by reference to inject_listener_closed

@tomaka
Copy link
Member

tomaka commented Apr 1, 2020

I took the liberty to finish the change, I hope that's ok.
It would be nice to get this PR in the next release, and this next release has already been waiting for a long time.

@tomaka tomaka requested a review from romanb April 1, 2020 11:50
@tomaka tomaka merged commit b1059cd into libp2p:master Apr 1, 2020
@tcharding
Copy link
Contributor Author

Please pass the error by reference to inject_listener_closed

Face palm - obviously. Thanks for finishing it. FTR feel free to take, modify, paraphrase, or drop anything I contribute. I'm just here to learn a bit and help out where it helps :)

Thanks.

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.

Pass the error to NetworkBehaviour::inject_listened_closed()
3 participants