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

fix(tcp): remove correct listener inTransport::remove_listener #3387

Merged
merged 3 commits into from
Jan 27, 2023

Conversation

jsantell
Copy link
Contributor

@jsantell jsantell commented Jan 25, 2023

Description

libp2p_tcp::Transport::remove_listener previously removed the first listener that did not match the provided ListenerId. This small fix brings it inline with other implementations.

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

@jsantell jsantell force-pushed the tcp-remove-listeners-fix branch from d41d6d3 to f182108 Compare January 25, 2023 19:49
@jsantell jsantell changed the title transports/tcp: Fix libp2p_tcp::Transport::remove_listener fix: transports/tcp: Fix libp2p_tcp::Transport::remove_listener Jan 25, 2023
@thomaseizinger thomaseizinger changed the title fix: transports/tcp: Fix libp2p_tcp::Transport::remove_listener fix(tcp): remove correct listener inTransport::remove_listener Jan 25, 2023
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Nice catch! Thanks for adding a test too!

Just one request about the changelog entry!

transports/tcp/CHANGELOG.md Outdated Show resolved Hide resolved
@jsantell jsantell force-pushed the tcp-remove-listeners-fix branch from f182108 to fbec93f Compare January 25, 2023 21:18
@jsantell
Copy link
Contributor Author

Thanks for the speedy feedback @thomaseizinger! Updated the changelog with your suggestion and a PR link

thomaseizinger
thomaseizinger previously approved these changes Jan 25, 2023
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix!

jsantell added a commit to subconsciousnetwork/noosphere that referenced this pull request Jan 26, 2023
* Adds `server::APIServer`, a web server for local interaction of name system node.
* Adds `server::HTTPClient` for interfacing with `server::APIServer`.
* `NameSystemClient` trait introduced, implemented by both `NameSystem`
  `and server::HTTPClient`.
* Uses my own fork of libp2p (0.51.0) with libp2p/rust-libp2p#3387 changes.
@jsantell jsantell force-pushed the tcp-remove-listeners-fix branch from fbec93f to 73c9636 Compare January 26, 2023 00:53
@mergify mergify bot dismissed thomaseizinger’s stale review January 26, 2023 00:54

Approvals have been dismissed because the PR was updated after the send-it label was applied.

thomaseizinger
thomaseizinger previously approved these changes Jan 26, 2023
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

No need to force-push / update the PR unless there are conflicts, the merge bot will handle it.

@thomaseizinger
Copy link
Contributor

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Jan 26, 2023

refresh

✅ Pull request refreshed

@mergify mergify bot dismissed thomaseizinger’s stale review January 27, 2023 03:42

Approvals have been dismissed because the PR was updated after the send-it label was applied.

thomaseizinger
thomaseizinger previously approved these changes Jan 27, 2023
mxinden
mxinden previously approved these changes Jan 27, 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.

Thank you @jsantell for both debugging and providing the patch!

@mergify mergify bot dismissed stale reviews from mxinden and thomaseizinger January 27, 2023 16:57

Approvals have been dismissed because the PR was updated after the send-it label was applied.

@mergify mergify bot merged commit d344406 into libp2p:master Jan 27, 2023
@mxinden
Copy link
Member

mxinden commented Jan 27, 2023

I am planning to cut v0.51.0 in two weeks. In case that is not fast enough, we might be able to do a patch release. Please comment in case that is important for you.

@jsantell
Copy link
Contributor Author

I am planning to cut v0.51.0 in two weeks. In case that is not fast enough, we might be able to do a patch release. Please comment in case that is important for you.

Thanks for the merge! v0.51.0 in two weeks will be plenty fast for us, thank you!

@jsantell jsantell deleted the tcp-remove-listeners-fix branch January 27, 2023 17:53
jsantell added a commit to subconsciousnetwork/noosphere that referenced this pull request Jan 27, 2023
* Adds `server::APIServer`, a web server for local interaction of name system node.
* Adds `server::HTTPClient` for interfacing with `server::APIServer`.
* `NameSystemClient` trait introduced, implemented by both `NameSystem`
  `and server::HTTPClient`.
* Uses an interim version of libp2p (0.51.0) with libp2p/rust-libp2p#3387 changes, until the major 0.51.0 release.
cdata pushed a commit to subconsciousnetwork/noosphere that referenced this pull request Jan 31, 2023
* Adds `server::APIServer`, a web server for local interaction of name system node.
* Adds `server::HTTPClient` for interfacing with `server::APIServer`.
* `NameSystemClient` trait introduced, implemented by both `NameSystem`
  `and server::HTTPClient`.
* Uses an interim version of libp2p (0.51.0) with libp2p/rust-libp2p#3387 changes, until the major 0.51.0 release.
@cdata
Copy link

cdata commented Feb 7, 2023

Hi @mxinden I work with @jsantell

We're eagerly anticipating the 0.51.0 release. Do you think that's on schedule to come out this week?

@mxinden
Copy link
Member

mxinden commented Feb 9, 2023

@jsantell and @cdata sorry for the delay here. I was on vacation and caught a cold afterwards. I will kick things off next week.

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.

4 participants