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

transports/webrtc: Implement message framing #10

Merged
merged 44 commits into from
Oct 12, 2022

Conversation

thomaseizinger
Copy link

@thomaseizinger thomaseizinger commented Sep 27, 2022

Description

This continues on #9 which I unfortunately couldn't push to. This PR is based off a branch in rust-libp2p now so @mxinden should definitely have push access and because it targets @melekes's PR he should have push access to :)

Links to any relevant issues

Open Questions

Open tasks

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 changed the title transports/webrtc/: Implement message framing transports/webrtc: Implement message framing Sep 27, 2022
Copy link
Owner

@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.

looks good so far 👍

@thomaseizinger
Copy link
Author

Writing the muxer test suite turns out to be a bit more complex than I thought it would. I'll see to dedicate most of tomorrow to that.

Might have to first write tests specific to this impl.

@melekes
Copy link
Owner

melekes commented Oct 10, 2022

Looks like the branch has diverged from anton/webrtc-transport. @thomaseizinger do you mind fixing this?

@thomaseizinger
Copy link
Author

Looks like the branch has diverged from anton/webrtc-transport. @thomaseizinger do you mind fixing this?

Yep, I should have time for this tomorrow! I got caught up trying to implement stream-resets properly on the other muxers so we can have a standardised test suite but that turns out to be a bigger effort.

transports/webrtc/Cargo.toml Outdated Show resolved Hide resolved
transports/webrtc/src/substream.rs Outdated Show resolved Hide resolved
transports/webrtc/src/substream.rs Show resolved Hide resolved
Copy link
Owner

@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.

@melekes melekes marked this pull request as ready for review October 11, 2022 08:15
@thomaseizinger
Copy link
Author

I also implemented sending of reset flags now for dropped substreams. I think this is now actually complete :)

@thomaseizinger
Copy link
Author

@mxinden Can you review this please?

@melekes melekes merged commit 225453b into melekes:anton/webrtc-transport Oct 12, 2022
@melekes
Copy link
Owner

melekes commented Oct 12, 2022

@mxinden I think you still should be able to review here or in main PR (whatever you prefer).

mergify bot pushed a commit to paritytech/smoldot that referenced this pull request Oct 13, 2022
Updates #2759
See melekes/rust-libp2p#10 and
libp2p/specs#412 (comment)

Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>
@mxinden
Copy link

mxinden commented Oct 14, 2022

I will review the main branch. Sorry for being a bottleneck here. Thanks @thomaseizinger for driving this all the way from the spec to the implementation! 🙏

@thomaseizinger thomaseizinger deleted the webrtc-message-framing branch October 14, 2022 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants