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): deprecate ConnectionHandlerEvent::Close #4714

Merged
merged 4 commits into from
Oct 24, 2023

Conversation

thomaseizinger
Copy link
Contributor

Description

We've already agreed that we want to remove this. There is no reason to not let our users know about this already through a deprecation. From the deprecation alone we can see how invasive this type parameter actually is, esp. compared to how little value it adds to the library.

Related: #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 24, 2023
swarm/CHANGELOG.md Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger merged commit e5cee61 into v0.52 Oct 24, 2023
70 checks passed
@thomaseizinger thomaseizinger deleted the feat/deprecate-close branch October 24, 2023 22:39
@thomaseizinger thomaseizinger restored the feat/deprecate-close branch October 24, 2023 22:39
@thomaseizinger
Copy link
Contributor Author

@mxinden What is the procedure again for getting this into master? I've released the tip of v0.52 now. Do I merge v0.52 into master or do I just cherry-pick the changes into a new PR towards master?

mergify bot pushed a commit that referenced this pull request Nov 2, 2023
This PR implements the long-awaited design of disallowing `ConnectionHandler`s 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.

Pull-Request: #4755.
@mxinden
Copy link
Member

mxinden commented Nov 3, 2023

@thomaseizinger you forgot to push the libp2p-swarm-v0.43.7 tag.

@mxinden
Copy link
Member

mxinden commented Nov 3, 2023

@mxinden What is the procedure again for getting this into master? I've released the tip of v0.52 now. Do I merge v0.52 into master or do I just cherry-pick the changes into a new PR towards master?

Generally I would prefer a merge over a cherry-pick. Though in this case, given that #4755 removes the deprecated items, I think it is fine as is.

@thomaseizinger
Copy link
Contributor Author

@thomaseizinger you forgot to push the libp2p-swarm-v0.43.7 tag.

I didn't have one locally so I made one now :)

@thomaseizinger
Copy link
Contributor Author

@mxinden What is the procedure again for getting this into master? I've released the tip of v0.52 now. Do I merge v0.52 into master or do I just cherry-pick the changes into a new PR towards master?

Generally I would prefer a merge over a cherry-pick. Though in this case, given that #4755 removes the deprecated items, I think it is fine as is.

I'll just manually update the changelog then.

@mxinden
Copy link
Member

mxinden commented Nov 3, 2023

@mxinden What is the procedure again for getting this into master? I've released the tip of v0.52 now. Do I merge v0.52 into master or do I just cherry-pick the changes into a new PR towards master?

Generally I would prefer a merge over a cherry-pick. Though in this case, given that #4755 removes the deprecated items, I think it is fine as is.

I'll just manually update the changelog then.

Whatever works best for you. I don't feel strongly here. Thank you!

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants