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: introduce libp2p-connection-limits connection management module #3386

Merged
merged 230 commits into from
Mar 21, 2023

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Jan 25, 2023

Description

This patch deprecates the existing connection limits within Swarm and uses the new NetworkBehaviour APIs to implement it as a plugin instead.

Related #2824.

Notes

This is a WIP in that it still needs tests but overall, this is pretty much what it will look like.

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

This is handled by `DialError` and `ListenFailure`.
@thomaseizinger
Copy link
Contributor Author

Use the new libp2p-swarm-test framework for this.

@thomaseizinger
Copy link
Contributor Author

Use the new libp2p-swarm-test framework for this.

To actually do this, I had to move connection_limits to a separate crate because it didn't work to depend on libp2p-swarm-test from libp2p-swarm. I think this is a tad cleaner anyway because we don't unnecessarily grow the libp2p-swarm API then.

@thomaseizinger thomaseizinger changed the title feat(swarm): implement connection limits using NetworkBehaviour feat: introduce libp2p-connection-limits connection management module Mar 11, 2023
@mergify
Copy link
Contributor

mergify bot commented Mar 12, 2023

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

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.

Looks good to me apart from suggestion to move to protocols/.

}
}

impl NetworkBehaviour for Behaviour {
Copy link
Member

Choose a reason for hiding this comment

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

We overload the string protocol with many meanings. The main one for in relation with the protocols/ folder being that all things in their implement NetworkBehaviour.

Given that libp2p_connection_limits::Behaviour implements NetworkBehaviour I think it should live in protocols/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main one for in relation with the protocols/ folder being that all things in their implement NetworkBehaviour.

We already have multiple utilities in libp2p-swarm that implement NetworkBehaviour without living under protocols/. To me, a libp2p "protocol" must have some form of network communication. This one here uses the dummy::ConnectionHandler so it will never perform any IO, hence I placed it in misc/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you know, I am not a massive fan of the directory structure anyway but putting this one into protocols/ actually feels wrong.

Protocols should be present on the wire, this one here is just an internal rust-libp2p concept.

misc/ is not a great name either because everything fits the description :)

With libp2p-identity merged, I'd like to push for the next step towards moving around the crate boundaries. Once we have all that in place, I think it makes sense to think about new directories.

Thoughts?

@@ -0,0 +1,3 @@
# 0.1.0 - unreleased
Copy link
Member

Choose a reason for hiding this comment

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

Nit pick, we recommend the below. I don't care which syntax we use, as long as we stay consistent. Makes my life creating the docs(CHANGELOG): Prepare xxx a bit easier.

Suggested change
# 0.1.0 - unreleased
# 0.1.0 [unreleased]

https://github.com/libp2p/rust-libp2p/blob/master/docs/release.md#development-between-releases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[unreleased] is a link reference that doesn't point to anything so I stopped using that. Can update the docs if you agree.

Copy link
Member

Choose a reason for hiding this comment

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

Never thought about that. Yes, patch would be appreciated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR here: #3615

@thomaseizinger
Copy link
Contributor Author

I am removing the dependency on #3615. Based on #3386 (comment), I am assuming that @mxinden is in favor of the new changelog syntax so I am using this here and we can iterate on the exact wording in #3615 after that.

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