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: introduce libp2p-stream #5027

Merged
merged 40 commits into from
Jan 16, 2024
Merged

feat: introduce libp2p-stream #5027

merged 40 commits into from
Jan 16, 2024

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Dec 24, 2023

Description

For a while now, rust-libp2p provided the request-response abstraction which makes it easy for users to build request-response based protocols without having to implement a NetworkBehaviour themselves. This PR introduces an alpha version of libp2p-stream: a NetworkBehaviour that directly gives access to negotiated streams.

In addition to complementing request-response, libp2p-stream also diverges in its design from the remaining modules by offering a clonable Control that provides async functions.

Resolves: #4457.

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
Copy link
Contributor Author

I've pushed an initial implementation. It was actually easier than I expected. Still need to review it myself once I've got fresher eyes though!

It also doesn't have any documentation yet although there is quite a bit I want to say about the design in regards to backpressure for example.

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.

This is great! The implementation is simple, the example is even simpler.

I have some suggestions but overall, from an architecture perspective, this looks good to me.

examples/stream/src/main.rs Outdated Show resolved Hide resolved
examples/stream/src/main.rs Outdated Show resolved Hide resolved
examples/stream/src/main.rs Outdated Show resolved Hide resolved
examples/stream/src/main.rs Outdated Show resolved Hide resolved
examples/stream/Cargo.toml Outdated Show resolved Hide resolved
examples/stream/src/lib.rs Outdated Show resolved Hide resolved
examples/stream/src/lib.rs Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor Author

@mxinden This is actually getting into a state where I am considering to make it a regular libp2p module but release it as an alpha first. That means we don't have to commit to the API right away and can likely get better feedback than just an example.

@mxinden
Copy link
Member

mxinden commented Dec 26, 2023

@mxinden This is actually getting into a state where I am considering to make it a regular libp2p module but release it as an alpha first. That means we don't have to commit to the API right away and can likely get better feedback than just an example.

Sounds good to me!

@thomaseizinger thomaseizinger changed the title feat: add example for a stream::Behaviour feat: introduce libp2p-stream Dec 27, 2023
@thomaseizinger
Copy link
Contributor Author

@mxinden: I've update this PR to get rid of the PeerControl API.

I decided that I don't want to implement any form of connection keep-alive. Instead, requesting a new stream should simply attempt to establish a new connection. I think that is overall more useful!

But without the PeerControl API, there is the chicken-egg problem of not knowing which peer a request is for before reading it from the channel. To solve that, I ended up using shared memory instead of channels. This allows each Control to always know the latest state of all connections and obtain a Sender for a particular connection atomically. To issue new dials, I used a channel to ensure we don't have a lock inside the poll function.

Curious to hear what you think of this design!

@thomaseizinger thomaseizinger marked this pull request as ready for review January 13, 2024 17:19
@thomaseizinger
Copy link
Contributor Author

@mxinden I decided to also move accept to the Control to be a) more consistent and b) give users more flexibility in how and when they listen on protocols.

@thomaseizinger
Copy link
Contributor Author

@mxinden I'd suggest to merge this with a failing semver job because I want to release it right away anyway.

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.

Looking forward for this to land! I expect this to be useful for many, especially those implementing a protocol on top of libp2p for the first time.

protocols/stream/README.md Outdated Show resolved Hide resolved
protocols/stream/src/handler.rs Show resolved Hide resolved
examples/stream/src/main.rs Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor Author

@rkuhn I resolved your comments to allow this to merge! :)

@mergify mergify bot merged commit b958897 into master Jan 16, 2024
72 of 74 checks passed
@mergify mergify bot deleted the feat/stream-behaviour branch January 16, 2024 03:59
umgefahren pushed a commit to p2p-industries/rust-libp2p that referenced this pull request May 20, 2024
For a while now, `rust-libp2p` provided the `request-response` abstraction which makes it easy for users to build request-response based protocols without having to implement a `NetworkBehaviour` themselves. This PR introduces an alpha version of `libp2p-stream`: a `NetworkBehaviour` that directly gives access to negotiated streams.

In addition to complementing `request-response`, `libp2p-stream` also diverges in its design from the remaining modules by offering a clonable `Control` that provides `async` functions.

Resolves: libp2p#4457.

Pull-Request: libp2p#5027.
umgefahren pushed a commit to p2p-industries/rust-libp2p that referenced this pull request May 20, 2024
For a while now, `rust-libp2p` provided the `request-response` abstraction which makes it easy for users to build request-response based protocols without having to implement a `NetworkBehaviour` themselves. This PR introduces an alpha version of `libp2p-stream`: a `NetworkBehaviour` that directly gives access to negotiated streams.

In addition to complementing `request-response`, `libp2p-stream` also diverges in its design from the remaining modules by offering a clonable `Control` that provides `async` functions.

Resolves: libp2p#4457.

Pull-Request: libp2p#5027.
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.

Add an example that documents how to build an "escape hatch" NetworkBehaviour that just hands out streams
3 participants