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): don't have ConnectionHandlers close connections #4755

Merged
merged 15 commits into from
Nov 2, 2023

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Oct 29, 2023

Description

This PR implements the long-awaited design of disallowing ConnectionHandlers to close entire connections. Instead, users should close connections via ToSwarm::CloseConnection from a NetworkBehaviour or - even better - from the Swarm via close_connection. A NetworkBehaviour also does not have a "full" view onto how a connection is used but at least it can correlate whether it created the connection via the ConnectionId. In general, the more modular and friendly approach is to stop "using" a connection if a particular protocol no longer needs it. As a result of the keep-alive algorithm, such a connection is then closed automatically.

Depends-on: #4745.
Depends-on: #4718.
Depends-on: #4749.
Related: #3353.
Related: #4714.
Resolves: #3591.

Notes & 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 added the internal-change Pull requests that make internal changes to crates and thus don't need to include a changelog entry. label Oct 29, 2023
@mxinden
Copy link
Member

mxinden commented Oct 29, 2023

Let me know once this is ready for a review. Direction looking good.

@thomaseizinger
Copy link
Contributor Author

Let me know once this is ready for a review. Direction looking good.

Needs all linked PRs to merge first before the build is green, otherwise ready for review :)

@thomaseizinger thomaseizinger marked this pull request as ready for review November 1, 2023 09:58
@thomaseizinger
Copy link
Contributor Author

@mxinden All dependencies of this PR have been merged so this is ready for review.

swarm/CHANGELOG.md Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger added this to the v0.53.0 milestone Nov 2, 2023
Copy link
Contributor

mergify bot commented Nov 2, 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.

Great to see this final step.

@mxinden mxinden added the send-it label Nov 2, 2023
@mergify mergify bot merged commit 0ef6feb into master Nov 2, 2023
71 checks passed
@mergify mergify bot deleted the feat/3591-remove-close branch November 2, 2023 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal-change Pull requests that make internal changes to crates and thus don't need to include a changelog entry. send-it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

swarm: Remove ConnectionHandler::Error
2 participants