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

core/muxing: Have functions on StreamMuxer take Pin<&mut Self> #2765

Merged
merged 12 commits into from
Aug 3, 2022
Merged

core/muxing: Have functions on StreamMuxer take Pin<&mut Self> #2765

merged 12 commits into from
Aug 3, 2022

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Jul 20, 2022 •
edited
Loading

Description

This is the idiomatic way of doing poll interface in Rust.

Links to any relevant issues

#2722
Depends on #2763.
Depends on #2764.
Depends on #2775.
Depends on #2776.

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
Introduce StreamMuxerExt trait
01eef20
@thomaseizinger
Have functions on StreamMuxer use Pin<&mut Self>
1096976 
This is the idiomatic way of defining `poll`-based interfaces in
Rust despite introducing a bit of inconvenience.

To work around this inconvenience, we also introduce a `StreamMuxerExt`
trait allows for better integration with async-await style code and
adds a `_unpin` variant to for each `poll` function of the `StreamMuxer`.
@thomaseizinger
Remove Mutex from libp2p-yamux implementation
df3b797 
Now that the `StreamMuxer` interface has `Pin<&mut Self>` receivers,
we can drop the `Mutex` from the yamux implementation.
@thomaseizinger thomaseizinger changed the title Introduce StreamMuxerExt trait core/muxing: Have functions on StreamMuxer take Pin<&mut Self> Jul 20, 2022
elenaf9
elenaf9 reviewed Jul 23, 2022
View reviewed changes
core/CHANGELOG.md Outdated Show resolved Hide resolved
core/src/muxing.rs Show resolved Hide resolved
swarm/src/connection.rs Outdated Show resolved Hide resolved
thomaseizinger and others added 4 commits July 23, 2022 19:46
@thomaseizinger @elenaf9
Update core/CHANGELOG.md
f16ee15 
Co-authored-by: Elena Frank <[email protected]>
@thomaseizinger
Have StreamMuxerExt::close consume self
cebef85 
The muxer is useless after polling `poll_close` to completion, thus
we can consume `self` in the extension trait method which allows us
to use the function within `libp2p_swarm::Connection`.
@thomaseizinger
Merge branch 'master' into feature/pin-mut-self-muxer
c3d69d9
@thomaseizinger
Fix compile errors and warnings
b82ff48
This was referenced Jul 25, 2022
Merged
Closed
mxinden
mxinden reviewed Jul 25, 2022
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.

🙏 thanks for the patch using Pin.

core/src/muxing.rs Show resolved Hide resolved
core/src/muxing.rs Show resolved Hide resolved
@thomaseizinger thomaseizinger marked this pull request as ready for review July 31, 2022 15:25
@thomaseizinger
Copy link
Contributor Author

@elenaf9 @mxinden This is ready for another review.

I believe this is useful despite the on-going discussion on the overall design because we will be using Pin anyway for poll function.

@mxinden mxinden mentioned this pull request Aug 3, 2022
Merged
4 tasks
mxinden
mxinden approved these changes Aug 3, 2022
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.

Wonderful to see this happening! 🙏

core/src/muxing.rs Show resolved Hide resolved
muxers/yamux/src/lib.rs Show resolved Hide resolved
@mxinden
Copy link
Member

mxinden commented Aug 3, 2022

@elenaf9 any objections to me merging here with the discussion on #2722 (comment) in mind?

I think we want to take Pin<&mut Self> no matter whether we go with 3 poll_ methods (poll_inbound, poll_outbound, poll_close) or one (poll).

melekes
melekes approved these changes Aug 3, 2022
View reviewed changes
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

@thomaseizinger thomaseizinger mentioned this pull request Aug 3, 2022
Closed
@thomaseizinger thomaseizinger merged commit 028dece into master Aug 3, 2022
@thomaseizinger thomaseizinger deleted the feature/pin-mut-self-muxer branch August 3, 2022 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxinden mxinden mxinden approved these changes

@melekes melekes melekes approved these changes

@elenaf9 elenaf9 elenaf9 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@thomaseizinger @mxinden @melekes @elenaf9