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

Wasm: add a transport that binds to a JS libp2p transport #974

Closed
tomaka opened this issue Feb 26, 2019 · 17 comments · Fixed by #1070
Closed

Wasm: add a transport that binds to a JS libp2p transport #974

tomaka opened this issue Feb 26, 2019 · 17 comments · Fixed by #1070
Assignees
Labels
difficulty:easy difficulty:moderate getting-started Issues that can be tackled if you don't know the internals of libp2p very well

Comments

@tomaka
Copy link
Member

tomaka commented Feb 26, 2019

Using wasm-bindgen, we can write an implementation of Transport that gets passed a JsValue which is expected to conform to https://github.com/libp2p/interface-transport
This way, we can use the existing js-libp2p transports if we compile Rust in the browser, instead of having to rewrite everything in Rust.

@tomaka tomaka added priority:oneday difficulty:easy difficulty:moderate getting-started Issues that can be tackled if you don't know the internals of libp2p very well labels Feb 26, 2019
@tomaka tomaka self-assigned this Feb 26, 2019
@tomaka
Copy link
Member Author

tomaka commented Feb 26, 2019

Oh well, someone who knows js-libp2p is probably better suited than me.
I can help on the Rust side though.

Alternatively, I can do it if someone is available to answer my questions about js-libp2p.

@tomaka tomaka removed their assignment Feb 26, 2019
@tomaka
Copy link
Member Author

tomaka commented Feb 26, 2019

Once done, we could remove the browser-specific code from libp2p-websocket.

@najamelan
Copy link

This might interest you. I made a small lib that let's you treat websockets as an tokio AsyncRead/AsyncWrite in wasm. No need to do anything more "browser specific". All you need is to compile to wasm, and be able to work with AsyncRead/AsyncWrite. The lib is very young, still needs review (unit tests fail in release mode, dunno why yet). It might get included in gloo at some point. And it's aimed at async-await, but if a version for futures 0.1 is wanted, it's not to hard to convert that and make a legacy version.

https://users.rust-lang.org/t/request-for-review-another-small-step-on-wasm-compatibility-asyncread-asyncwrite-over-websockets/26557

@dryajov
Copy link
Member

dryajov commented Mar 25, 2019

Hey @tomaka this sounds really cool and something we'll need at some point. I can give you a hand with the JS side - let me know how you want to do this.

@tomaka
Copy link
Member Author

tomaka commented Mar 25, 2019

@najamelan Thanks for the help. I also have a working implementation of WebSockets-in-JavaScript-in-Rust in #970, but the approach of this PR here is I think better (provided that the API of the JavaScript libraries doesn't change).

@tomaka
Copy link
Member Author

tomaka commented Mar 25, 2019

@dryajov I've had a few issues:

  • It's not possible to cancel an outgoing JavaScript dialing attempt, which makes everything more complicated because we have to keep resources alive until the callback gets called (and hope it gets called, otherwise there's a memory leak).
  • How do I know that getAddrs() on a listener has finished calling the callback passed to it? Or is it infinite?
  • The fact that closing a listener kills all the active connections that were created through it is extremely surprising to me and not compatible with rust-libp2p's API.
  • If I have a "source" (as in pull-stream's sources), and I call source(null, cb); source(true);, can cb be called afterwards?
  • Does cancelling a read on a source, for example by calling source(-1), close the source forever? Or can I call source(cb) again in the future?

@dryajov
Copy link
Member

dryajov commented Mar 25, 2019

Hey @tomaka

It's not possible to cancel an outgoing JavaScript dialing attempt, which makes everything more complicated because we have to keep resources alive until the callback gets called (and hope it gets called, otherwise there's a memory leak).

This should be possible by calling the .close method on the returned connection object.

It looks something like this, the .dial method takes a callback argument that will be called only after the connection is established, meanwhile, dial also returns a connection object (a pull-stream stream), this connection can be closed at any time. if the transport supports cancelations, it will either cancel an ongoing dial attempt, or close the existing connection. In the case of the websocket transport, it uses pull-ws, you can see how it handles closes here - https://github.com/pull-stream/pull-ws/blob/master/client.js#L22.

So the flow in js is something like this:

const conn = ws.dial([mas], options, () => {
// the connection is ready to use
})
...
// close an established connection or cancel an ongoing dial attempt
conn.close(() => {...})

How do I know that getAddrs() on a listener has finished calling the callback passed to it? Or is it infinite?

The callback should be always called, either with an error if no addresses are available or with the resolved addresses, it should never hang/not execute the callback. I assume you're seeing this, in that case it's either a bug is the transport, or something else related to js/webassembly interop.

The fact that closing a listener kills all the active connections that were created through it is extremely surprising to me and not compatible with rust-libp2p's API.

Yes, this is how it works right now unfortunately. A fix was started some time ago, but I'm not sure how far along this effort is and weather it's a priority ATM. Here is a (lengthy) tread for a bit of background.

If I have a "source" (as in pull-stream's sources), and I call source(null, cb); source(true);, can cb be called afterwards?

I believe the answer is no, if I'm reading this correctly.

Does cancelling a read on a source, for example by calling source(-1), close the source forever? Or can I call source(cb) again in the future?

Yes, it closes it forever. Once the source ended, it should not be usable anywhere else.

What is the specific use case you looking at with pull streams?


Here is some background on pull streams:

If you call source(true), it signals the source to abort reading and exit, it should not be usable after that. This is the intended behavior, otherwise the source isn't correctly implemented. Also, a properly implemented sync, should not continue using the source after it has ended.

Just as an example, here are the two of the most used sources values and syncs drain:

The values source will read one element of an array until no more elements are left (L18), at which point it will call cb with true, drain on the other hand, calls a source until it returns true in it's cb (also L18).

The way this looks in code is like this:

const pull = require('pull-stream')

pull(
  pull.values([1, 2, 3, 4]), // drain calls the closure returned from values repeatedly until it "returns" true in the read callback
  pull.drain((val) => {
    console.log(val) // prints 1234
    }, 
    (err) => console.log))

Hope this answers it for pull-streams.

@tomaka
Copy link
Member Author

tomaka commented Mar 25, 2019

That's very helpful, thanks!

The callback should be always called, either with an error if no addresses are available or with the resolved addresses, it should never hang/not execute the callback. I assume you're seeing this, in that case it's either a bug is the transport, or something else related to js/webassembly interop.

That seems to be a misunderstanding from myself. I was assuming that the callback was called once per address. Are you saying that the callback is only called once with an array of addresses?

What is the specific use case you looking at with pull streams?

I need to implement the pull streams interface on the Rust side as well (because writing data is done by passing a source to a sink), and I'm trying to make sure that I'm doing this correctly.

@tomaka
Copy link
Member Author

tomaka commented Mar 25, 2019

For what it's worth, I've got some code in #970 that is working fine on the dialing side.
In order to implement the listening side correctly, however, we would need the changes described in #794 (comment)

@dryajov
Copy link
Member

dryajov commented Mar 25, 2019

That seems to be a misunderstanding from myself. I was assuming that the callback was called once per address. Are you saying that the callback is only called once with an array of addresses?

Yep, it should be called once with an array of multiaddrs.

I need to implement the pull streams interface on the Rust side as well (because writing data is done by passing a source to a sink), and I'm trying to make sure that I'm doing this correctly.

I'm not sure what sort of interop Rust has with js streams, but it is possible to convert between node streams and pull streams - https://github.com/pull-stream/stream-to-pull-stream and https://github.com/pull-stream/pull-stream-to-stream. Maybe rewriting pull streams in rust is not necessary? Here is some more background on pull streams ipfs/js-ipfs#362.

One more thing, in the case of the WS transport specifically, in the browser, only the dialer is used, the listening part is ignored on libp2p boot. Sadly we can't listen for websocket connections in the browsers.

I was thinking about it a little more and I wonder if directly using javascript transports in Rust is the right way to go about it? For one the implementations between Rust and JS are quite different, on the other hand, the transports are really thin wrappers around some lower level lib. In the browser it falls back to whatever underlying primitive there is for it (i.e. https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API).

So I wonder if it's worth the trouble of trying to make things interop or just use the js implementation as inspiration but fork wherever it makes sense from Rust's standpoint?

@dryajov
Copy link
Member

dryajov commented Mar 25, 2019

I'll take a closer look at #970 and try to lend a hand wherever I can 👍

@tomaka
Copy link
Member Author

tomaka commented Mar 26, 2019

So I wonder if it's worth the trouble of trying to make things interop or just use the js implementation as inspiration but fork wherever it makes sense from Rust's standpoint?

My objective is to make rust-libp2p work in the browser. There's only one possible way to do this: by interfacing with some JavaScript code. I can either interface with the browser/nodejs's APIs, or I can interface with the js-libp2p transports. Doing the latter would be a huge time saver.

@dryajov
Copy link
Member

dryajov commented Mar 26, 2019

Yeah, totally get the intend of this effort. My point is that there is lots of js-libp2p sepcifics in how this transports are implemented, for example pull-streams, that decision was based mostly on the limitations around node streams, but it does introduce a requirement for Rust to implement it. Another thing, which you brought up is the close functionality that drops all connections, which is most likely gonna take some time to complete.

What I'm thinking is, what of we'd take the JS implementations, fork them and tweak to work with Rust, rather than adapting/implementing the Rust parts.

@dryajov
Copy link
Member

dryajov commented Mar 26, 2019

For example the WS transport - we can expose an interface that's easiest to consume for Rust, but keep the internals mostly as is, i.e. continues using pull-ws a internally.

The pros as I see it are - a) no need to implement missing Js parts in rust (pull-streams), b) the rust transport interface doesn't have to accommodate for js-libp2p specifics, c) connection management can be done in a way that is more convenient for the rust implementation

Cons, you end up with rust specific Js transports. Maybe not such a biggie given how different both implementations are

@tomaka
Copy link
Member Author

tomaka commented Apr 11, 2019

Thinking about it, the right solution is probably to write some bindings on the Rust side corresponding to what the Rust expects, and write a compatibility layer in JavaScript.

@tomaka
Copy link
Member Author

tomaka commented Apr 11, 2019

Thinking about it, the right solution is probably to write some bindings on the Rust side corresponding to what the Rust expects, and write a compatibility layer in JavaScript.

The reason behind this is that writing JavaScript from within Rust is incredibly painful, as one has to manually handle all the possible corner cases (what if a method doesn't exist? what if this is null? what if this callback is called twice despite the documentation saying it should only be called once? and so on). By writing a compatibility layer in JavaScript, we delegate all these concerns to the code that uses rust-libp2p instead of having them in rust-libp2p itself.

Having a compatibility layer also means that we decouple WASM from JavaScript, which is never a bad thing. The JsTransport would become an ExternalTransport.

@dryajov
Copy link
Member

dryajov commented Apr 12, 2019

As I stated before, I would go as far as not trying to interface with the Js transports themselves. The Js transports are mostly wrappers around some lower level primitive that are in place to make it easier and more ergonomic to consume from Js itself, it would be a mistake to wrap around the wrapper.

@ghost ghost assigned tomaka Apr 18, 2019
@ghost ghost added the in progress label Apr 18, 2019
@ghost ghost removed the in progress label Apr 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty:easy difficulty:moderate getting-started Issues that can be tackled if you don't know the internals of libp2p very well
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants