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

feat(swarm)!: introduce ListenError #3375

Merged
merged 16 commits into from
Jan 26, 2023
Merged

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Jan 24, 2023

Description

In case an error happens for an outgoing connection, Pool reports an OutgoingConnectionError. This one is mapped to a DialError and reported via SwarmEvent::OutgoingConnectionError and FromSwarm::DialFailure.

For incoming connections, we didn't quite do the same thing. For one, SwarmEvent::IncomingConnectionError directly contained a PendingInboundConnectionError. Two, FromSwarm::ListenFailure did not include an error at all.

With this patch, we now introduce a ListenError enum which we use in SwarmEvent::IncomingConnectionError and we pass a reference to it along in FromSwarm::ListenFailure.

Notes

In #3254, I am going to introduce a Denied variant in both DialError and ListenError which contains the error that a NetworkBehaviour denied a connection with.

Links to any relevant issues

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@thomaseizinger thomaseizinger changed the title feat(swarm): introduce ListenError feat(swarm)!: introduce ListenError Jan 24, 2023
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Change looks good to me.

@@ -1677,6 +1683,68 @@ impl error::Error for DialError {
}
}

/// Possible errors when upgrading an inbound connection.
#[derive(Debug)]
pub enum ListenError {
Copy link
Member

Choose a reason for hiding this comment

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

Mentioning it here, though not directly related: I always confuse LIstenError, signaling a failure on upgrading an inbound connection, with the failure of listening on a new address. Anyone else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A lot of naming could be improved here :)

I believe this is consistent with the naming of DialError but I agree that something like InboundConnectionError and OutboundConnectionError would probably be better.

I'll see to tidy up these when we remove the connection limits and banned variants.

Base automatically changed from 2824-remove-unused-connection-error to master January 26, 2023 08:10
@thomaseizinger thomaseizinger marked this pull request as ready for review January 26, 2023 08:12
@mergify
Copy link
Contributor

mergify bot commented Jan 26, 2023

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

@mergify
Copy link
Contributor

mergify bot commented Jan 26, 2023

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

@mergify mergify bot merged commit d1336a7 into master Jan 26, 2023
@mergify mergify bot deleted the 2824-introduce-listen-error branch January 26, 2023 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants