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(webrtc): add WebRTC for WASM environments #4248

Merged
merged 256 commits into from
Sep 17, 2023

Conversation

DougAnderson444
Copy link
Contributor

@DougAnderson444 DougAnderson444 commented Jul 24, 2023

Description

This PR implements Transport for WebRTC for browsers by using web-sys. Only the webrtc-direct spec is implemented. The webrtc spec for connecting two browsers with each other is left to a future PR.

Related: libp2p/specs#475.
Related #2617.
Supersedes: #4229.

Attributions

Co-authored-by: Thomas Eizinger thomas@eizinger.io

Notes & open questions

I wanted to put this here so that if there are any others working towards Rust WebRTC Transport in the browser they will see this started. 💯no-silos🕳️

✔This PR is in the process of being finalized & reviewed for merging (hopefully "Soon" -- it's a big PR).

ToDo

MVP

  • It handshakes
  • Interop tests passing
  • Browser-to-Server
  • Add Example

Enhancements

  • Audit Fingerprint, SDP, Noise to determine what can be extracted to common utils crate
  • Finalize design of common utils create public api (libp2p-webrtc-utils)
  • Audit deps to ensure old ones are vetted out

Future PR (See: #4389)

  • Example works in Chrome and Firefox
  • Browser-to-Browser (might save for a follow-on PR)
  • Investigate handling of dropped listeners for the browser (onclose)
  • Unknown reason why ip4 fail vs ip6 succeeding in same local machine interop testing for webrtc

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

@DougAnderson444 DougAnderson444 mentioned this pull request Jul 24, 2023
4 tasks
@mxinden mxinden linked an issue Jul 28, 2023 that may be closed by this pull request
@mxinden mxinden mentioned this pull request Jul 28, 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.

Thank you for pushing this effort forward!

I made some high-level comments regarding the design, no deeper look yet :)

transports/webrtc/src/common/substream/state.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@DougAnderson444
Copy link
Contributor Author

Ok I have Noise questions / clarifications:

The WebRTC Direct spec says:

Why does B start the Noise handshake and not A?

Given that WebRTC uses DTLS 1.2, B is the one that can send data first.

What data are we referring to? Does the first "empty" count as first data? Or does first data exclude empty and really means the first identity sent, which would actually use the DTLS 1.2? If it's first identity, then does the browser send the first message, which contains no data since it's actually empty?

I ask because in Noise the initiator sends first, but it is an empty send

handshake::send_empty(&mut state).await?;
handshake::recv_identity(&mut state).await?;

and then of course the other option in the reverse direction:

handshake::recv_empty(&mut state).await?;
handshake::send_identity(&mut state).await?;

From the browser perspective we are dialing the server, which I would expect to be an upgrade_outbound but if the server needs to "initiate", should the browser be awaiting upgrade_inbound instead so the server can send the first empty?

Thanks! 😀

@thomaseizinger
Copy link
Contributor

Does this module help? https://github.com/libp2p/rust-libp2p/blob/master/transports/webrtc/src/tokio/upgrade/noise.rs

Essentially, the upgrades are "reversed", i.e. the connection initiator uses the inbound noise upgrade and vice versa. This seems like something that we shouldn't duplicate between the two implementations so we might after all need that libp2p-webrtc-utils/common crate :)

@thomaseizinger
Copy link
Contributor

@thomaseizinger I noticed you removed the Closure .forget() calls in poll_data_channel.rs which do the following (excerpt):

When a Closure is dropped it will release the Rust memory and invalidate the associated JS closure, but this isn’t always desired. Some callbacks are alive for the entire duration of the program or for a lifetime dynamically managed by the JS GC. This function can be used to drop this Closure while keeping the associated JS function still valid.

I thought I'd ask your thoughts on RS-JS memory management for this. My interpretation was by using forget we were delinking the Closure from Rust and handing over memory management of those Closure to JS so it was appropriate (they use forget in the WebRTC DataChannel example). That way when the Closure goes out of scope in Rust (wasm) it is still valid in JS (though I could be completely wrong here Chrome still seems to function fine, but in Firefox one of the many issues appears to be a Closure failure issue, and I was wondering if it might be linked to the removal of forget())

What made me rewrite this is the following sentence in the docs:

By default this function will leak memory.

As far as I understand, this would never be cleaned up properly if we use .forget(). We only need the closure for as long as the connection is alive so keeping it around and dropping it together with Connection seemed the correct thing to do.

Maybe leaking the memory and having the browser do GC could also work. After all, the connection should also get cleaned up there eventually. However, I think because we have an opportunity to correctly track for how long the closure should be alive, I opted for not calling forget().

Regarding the example: My interpretation would be that it is just that, an example, and thus not entirely clean in a way that you'd do it for a production application.

@mergify

This comment was marked as resolved.

@mergify

This comment was marked as resolved.

@mergify

This comment was marked as resolved.

@mxinden
Copy link
Member

mxinden commented Sep 16, 2023

Judging by your recent merges into this pull request, I am guessing you want this merged? I resolved an old conversation. Still needs an approval.

@thomaseizinger
Copy link
Contributor

Judging by your recent merges into this pull request, I am guessing you want this merged? I resolved an old conversation. Still needs an approval.

I just wanted CI to be fully green. This is ready to merge from my end but I wrote some of the code too so would appreciate a review from your end!

@DougAnderson444
Copy link
Contributor Author

Sorry for being a bit MIA (missing in action) this week, we've had a hurricane, lost power, I got a new machine, and I've switched to neovim (oh man, the learning curve) all in one week (in addition to everything else!).

I finally reverted the Flag::Reset commit that @thomaseizinger and I were discussing earlier, so I believe we are all caught up and ready to merge?!

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 pull request looks good to me.

Great work @DougAnderson444 and @thomaseizinger!

@mergify mergify bot merged commit f5e644d into libp2p:master Sep 17, 2023
69 checks passed
@mxinden
Copy link
Member

mxinden commented Sep 17, 2023

@DougAnderson444 @thomaseizinger I think this is a huge step. Does either of you want to do a quick post in the #libp2p-implementers channel, inviting folks to try out the new example? Happy to do it myself, but I don't want to steal your thunder!

In addition, though that is a bit more involved, I am sure many people would enjoy a short blog post on https://github.com/libp2p/blog/.

@thomaseizinger
Copy link
Contributor

@DougAnderson444 @thomaseizinger I think this is a huge step. Does either of you want to do a quick post in the #libp2p-implementers channel, inviting folks to try out the new example? Happy to do it myself, but I don't want to steal your thunder!

@DougAnderson444 Please post if you are keen, you got this off the ground!

In addition, though that is a bit more involved, I am sure many people would enjoy a short blog post on https://github.com/libp2p/blog/.

I am happy to work on a post :)

Something along the lines of "fullstack Rust libp2p"?

@DougAnderson444
Copy link
Contributor Author

I would be honored to post this merge announcement!

thomaseizinger added a commit that referenced this pull request Sep 21, 2023
This PR implements `Transport` for WebRTC for browsers by using web-sys. Only the `webrtc-direct` spec is implemented. The `webrtc` spec for connecting two browsers with each other is left to a future PR.

Related: libp2p/specs#475.
Related #2617.
Supersedes: #4229.

Pull-Request: #4248.

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
@mxinden
Copy link
Member

mxinden commented Sep 22, 2023

Something along the lines of "fullstack Rust libp2p"?

Sounds catchy 👍

@DougAnderson444 DougAnderson444 deleted the websys-rtc branch September 22, 2023 11:41
@thomaseizinger
Copy link
Contributor

Something along the lines of "fullstack Rust libp2p"?

Sounds catchy 👍

Created an issue for the blog post here: libp2p/blog#113

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