feat: add websocket transport for browser environment based on web-sys

[vincev]

Description

Resolves #3611.

Notes & open questions

To enable this feature in the example:

[dependencies.libp2p]
version = "0.51"
path = "../../rust-libp2p/libp2p"
features = ["floodsub", "macros", "mplex", "noise", "wasm-bindgen", "websys-websocket"]

I tested the changes with the wasm-p2p-chat example but running tests automatically is not easy as it requires a headless browser.

Note I didn't remove wasm-ext as it looks from crates.io that there are people using it, maybe we can deprecate it.

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 (tested with chat example)
• A changelog entry has been made in the appropriate crates

[thomaseizinger]

Note I didn't remove wasm-ext as it looks from crates.io that there are people using it, maybe we can deprecate it.

I only see two dependents. One is our meta crate and the other one hasn't see a release in two years and depends on a really old version. I suspect it is unused.

To make things easier in case someone does depend on it, we should deprecate it first and only remove it later.

[thomaseizinger]

Thank you! I've left some comments and questions :)

[thomaseizinger]

We need to return Poll::Ready eventually here, otherwise we will never be able to close these streams. I think we should:

• Check if we are closed, if yes return Poll::Ready
• If not and we are also not closing, call Websocket::close and set a closing flag to true
• If we are closing, return Poll::Pending and set a waker
• Once we are called in on_close, call the waker

[thomaseizinger]

Should this perhaps be an enum instead of multiple flags? As far as I can tell, these are mutually exclusive.

[vincev]

Thanks Thomas these are very helpful comments, I'll make the changes in the next few days.

[thomaseizinger]

Let me know when this is ready for another review!

[vincev]

Let me know when this is ready for another review!

Thanks Thomas, I have renamed the package as you suggested to libp2p-websys-websocket so maybe you may want to grab the crates.io package.

I am a bit busy at the moment but I'll try to get more done this weekend, I'll ping you once the code is ready for another review.

[thomaseizinger]

Let me know when this is ready for another review!

Thanks Thomas, I have renamed the package as you suggested to libp2p-websys-websocket so maybe you may want to grab the crates.io package.

Thanks for the suggestion, done now: https://crates.io/crates/libp2p-websys-websocket

[mxinden]

This looks promising. Thanks for the work!

[mxinden]

Let me know when this is ready for another review!

Thanks Thomas, I have renamed the package as you suggested to libp2p-websys-websocket so maybe you may want to grab the crates.io package.

Thanks for the suggestion, done now: https://crates.io/crates/libp2p-websys-websocket

Thanks @thomaseizinger. If I recall correctly, members of a GitHub team can not add other people as owners on crates.io. For the sake of consistency, given that I am an owner of all libp2p-* crates, would you mind adding me as well?

[thomaseizinger]

Let me know when this is ready for another review!

Thanks Thomas, I have renamed the package as you suggested to libp2p-websys-websocket so maybe you may want to grab the crates.io package.

Thanks for the suggestion, done now: crates.io/crates/libp2p-websys-websocket

Thanks @thomaseizinger. If I recall correctly, members of a GitHub team can not add other people as owners on crates.io. For the sake of consistency, given that I am an owner of all libp2p-* crates, would you mind adding me as well?

Done.

[vincev]

@thomaseizinger thank you for helping with this, I have tried the new version but it looks that if you connect from the browser and then kill the server the client doesn't get a disconnection event.

With the old version if I connect send hello and then kill the server I get this:

but with the new version nothing happen, I can see that if we always set the write waker like:

    fn poll_write(
        self: Pin<&mut Self>,
        cx: &mut Context<'_>,
        buf: &[u8],
    ) -> Poll<Result<usize, io::Error>> {
        let mut shared = self.shared.lock();
        shared.write_waker = Some(cx.waker().clone());

then it works but I am not sure if that is the right thing to do.

[thomaseizinger]

@vincev Thank you for testing!

I did run your chat app as well after my refactorings but I didn't test the disconnection. Let me look into it.

[thomaseizinger]

then it works but I am not sure if that is the right thing to do.

Well, technically it is overkill but it also doesn't hurt per se. Generally, wakers should only be requested when necessary, otherwise, it is hard to understand when and why we are meant to wake a task.

Looking at the code now, something can't be quite right with what we implemented. At the moment, we don't have a branch that can return Pending if we cannot write data. Thus, it seems that the only way this can currently work is if send_with_u8_array would block? That is not right.

[thomaseizinger]

Wow, the documentation says that the browser will simply close the connection if we overload it with too much data haha: https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/send

So apparently, the solution is to just choose some buffer size and check periodically if bufferedAmount has lowered itself and then try again.

I am currently trying to clear out our backlog for the next breaking release so I don't have much capacity to implement this unfortunately. As it stands, the current solution isn't great because we don't have good backpressure. A client will simply send as much data as it can and if the browser can't keep up, it will close the connection :(

[mxinden]

Sorry for not engaging here any earlier.

Overall this looks good to me. Though, unless I am missing something, the above documented issue around bufferedAmount is a show-stopper unless addressed.

I did a quick scan over the js-libp2p code-base but couldn't find the corresponding handling there.

Maybe @achingbrain or @MarcoPolo do you know where js-libp2p is handling WebSocket's bufferedAmount or WebRTC's bufferedAmount?

[thomaseizinger]

Overall this looks good to me. Though, unless I am missing something, the above documented issue around bufferedAmount is a show-stopper unless addressed.

Yes I see it the same way.

[vincev]

Overall this looks good to me. Though, unless I am missing something, the above documented issue around bufferedAmount is a show-stopper unless addressed.

Yes this looks like a problem, probably for a simple chat example app like the one I wrote a simple websocket will do, hopefully you can reuse some of this code in the future when you expand libp2p WASM capabilities.

[thomaseizinger]

@vincev Did you delete the branch? I'd actually like to have this merged eventually, I just haven't had the time yet to finish it :)

[vincev]

@vincev Did you delete the branch? I'd actually like to have this merged eventually, I just haven't had the time yet to finish it :)

Oh gosh I interpreted yours and Max comment as a big blocker from websys for the implementation. Let me see if I can restore it.

[thomaseizinger]

@vincev Did you delete the branch? I'd actually like to have this merged eventually, I just haven't had the time yet to finish it :)

Oh gosh I interpreted yours and Max comment as a big blocker from websys for the implementation. Let me see if I can restore it.

It is a blocker for merging as is and the fix isn't pretty because it involves an arbitrary buffer limit and a timer but it is doable so it is not a fundamental blocker :)

[vincev]

It is a blocker for merging as is and the fix isn't pretty because it involves an arbitrary buffer limit and a timer but it is doable so it is not a fundamental blocker :)

Okay got it, I have raised a ticket with github I'll let you know as soon as is back.

[vincev]

@thomaseizinger repo is back.

[mxinden]

@vincev thank you. Sorry for not being clear. Your work here will still be useful!

[vincev]

@mxinden no problem at all, happy to help.

[vincev]

I am going to close this PR as I am not doing any work on this.

[thomaseizinger]

I've restored a copy of this in #4102. @vincev you will be correctly attributed once we merge that one :)

[mergify]

⚠️ The sha of the head commit of this PR conflicts with #4102. Mergify cannot evaluate rules on this PR. ⚠️

[vincev]

I've restored a copy of this in #4102. @vincev you will be correctly attributed once we merge that one :)

Thank you @thomaseizinger.