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.

Sign up for GitHub

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

Jump to bottom

refactor(identify): use ReadyUpgrade for {In,Out}boundUpgrade #4563

Merged
merged 13 commits into from
Sep 28, 2023
Merged

refactor(identify): use ReadyUpgrade for {In,Out}boundUpgrade #4563

merged 13 commits into from
Sep 28, 2023

Conversation

dgarus
Copy link
Contributor

@dgarus dgarus commented Sep 26, 2023 •
edited by thomaseizinger
Loading

Description

Related: #2863.

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

@dgarus
Copy link
Contributor Author

dgarus commented Sep 26, 2023

@thomaseizinger
Hello, Thomas!

Please take a look.

thomaseizinger
thomaseizinger reviewed Sep 26, 2023
View reviewed changes
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.

Thanks! That looks quite clean already, only a few comments :)

protocols/identify/src/handler.rs Outdated Show resolved Hide resolved
protocols/identify/src/handler.rs Outdated Show resolved Hide resolved
protocols/identify/src/handler.rs Outdated Show resolved Hide resolved
@dgarus
Copy link
Contributor Author

dgarus commented Sep 27, 2023

@thomaseizinger
Hi!
Thanks for the review, I fixed your comments.
There are a few moments to pay close attention to:

  • the capacity of the single FutureSet -   MAX_CONCURRENT_STREAMS_PER_CONNECTION
  • the logic of the connection_keep_alive method

@dgarus
Copy link
Contributor Author

dgarus commented Sep 27, 2023

@thomaseizinger
I found a mistake in the PR: need to have another way to handle Poll::Ready(Ok(Err(e))).
I'll fix this tomorrow.

mxinden
mxinden reviewed Sep 27, 2023
View reviewed changes
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.

Two comments. Otherwise this looks good to me. Thank you for the work!

protocols/identify/src/handler.rs Outdated Show resolved Hide resolved
protocols/identify/src/handler.rs Outdated Show resolved Hide resolved
thomaseizinger
thomaseizinger reviewed Sep 28, 2023
View reviewed changes
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.

Thank you!

A few suggestions around naming consistency and idiomatic Rust :)

protocols/identify/src/handler.rs Outdated Show resolved Hide resolved
protocols/identify/src/handler.rs Outdated Show resolved Hide resolved
protocols/identify/src/handler.rs Outdated Show resolved Hide resolved
protocols/identify/src/protocol.rs Outdated Show resolved Hide resolved
protocols/identify/src/protocol.rs Outdated Show resolved Hide resolved
protocols/identify/src/protocol.rs Outdated Show resolved Hide resolved
protocols/identify/src/protocol.rs Outdated Show resolved Hide resolved
This was referenced Sep 28, 2023
Closed
Open
thomaseizinger
thomaseizinger approved these changes Sep 28, 2023
View reviewed changes
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.

Thank you! Turned out quite clean and the diff is negative as well which is always good :)

@thomaseizinger thomaseizinger changed the title refactor(identify): use ReadyUpgrade for {In,Out}boundUpgrade refactor(identify): use ReadyUpgrade for {In,Out}boundUpgrade Sep 28, 2023
@mergify mergify bot merged commit ecdd0ff into libp2p:master Sep 28, 2023
74 checks passed
@dgarus
Copy link
Contributor Author

dgarus commented Sep 29, 2023

@thomaseizinger @mxinden

Thank you so much for the review!

@dgarus dgarus deleted the 2863-identify-2 branch September 29, 2023 09:29
@dhuseby dhuseby mentioned this pull request Oct 3, 2023
Merged
5 tasks
@jxs jxs mentioned this pull request Oct 30, 2024
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomaseizinger thomaseizinger thomaseizinger approved these changes

@mxinden mxinden Awaiting requested review from mxinden

Assignees
No one assigned
Labels
send-it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dgarus @thomaseizinger @mxinden