Skip to content

Add wasm-ext-transport #1070

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

Merged
merged 13 commits into from
Apr 25, 2019
Merged

Add wasm-ext-transport #1070

merged 13 commits into from
Apr 25, 2019

Conversation

tomaka
Copy link
Member

@tomaka tomaka commented Apr 18, 2019

Close #1004
Close #974

Instead of trying to match the API of js-libp2p, we just expose the API of rust-libp2p to the WASM outside world. A compatibility layer with js-libp2p may then be written in the future, but more like we would write an implementation that directly uses WebSockets or WebRTC for example.

@ghost ghost assigned tomaka Apr 18, 2019
@ghost ghost added the in progress label Apr 18, 2019
@tomaka tomaka mentioned this pull request Apr 18, 2019
@tomaka
Copy link
Member Author

tomaka commented Apr 19, 2019

@jacogr, could you give me your opinion on this JavaScript API, or if you don't have time ping someone who could do this?
I don't really know what consists in idiomatic JavaScript. I don't want programmers to look at this API and think "the person who designed this is stupid/incompetent".

The API is the following:

  • The main object that must be passed is a "transport" and has two methods: dial and listen_on.

  • dial(addr) -> Promise gets passed a string and must return a Promise that succeeds if we managed to reach the remote. The object that the Promise yields is a "connection".

  • listen_on(addr) -> Promise gets passed a String and must return a Promise that yields a "listen event".

  • A "listen event" has 4 fields: new_addrs, an array of strings, expired_addrs, an array of strings, new_connections, an array of "connection events", and next_event, a Promise that will yield the next "listen event".

  • A "connection event" has 3 fields: connection, the connection in question, observed_addr, a string, and local_addr, a string.

  • A "connection" has four methods: read, write, shutdown, close.

  • read() -> Promise returns a Promise that yields either an ArrayBuffer containing the data we received, or null on EOF. read is guaranteed to be called again only after the last returned Promise has succeeded, and not before.

  • write(data) -> Promise sends the data and returns a Promise that succeeds when we are ready to write more data. write is guaranteed to be called again only after the last returned Promise has succeeded, and not before.

  • shutdown() is instant and closes the writing side of the connection (can be implemented as a no-op). Afterwards, write is guaranteed to never be called again.

  • close() is instant and closes the connection entirely. Afterwards, no other method will be called.

Let me know if this looks idiomatic, or if there are changes that I should make.
I quite like the fact that listen_on returns a Promise that returns an event object that contains a Promise to the next event, but I didn't do the same for read or write.

One remaining issue I have is that in dial I would like to differentiate a situation where we fail to dial because we can't parse the multiaddress, and a situation where we fail to dial because the remote is unreachable. In the first situation, the caller should try a different transport.
However I don't know how to represent that in JavaScript in an idiomatic way.

@jacogr
Copy link

jacogr commented Apr 20, 2019

Traveling a bit, however will take a peek and see if it is usable as-is for the JS client.

From a “does it make sense as an API”, @amaurymartiny could also have a peek, more eyes are always better.

Generally for “multiple stuff will appear” either callbacks or eventemitters are generally be used (or even iterators)

@tomaka
Copy link
Member Author

tomaka commented Apr 20, 2019

see if it is usable as-is for the JS client.

Oh, there's probably a confusion. This API would have to be implemented in JavaScript (using WebSockets for example), and the object that implements it gets passed to the Rust code.
It's the link between the Rust and the networking that the "operating system" provides, which would generally mean the browser.

Generally for “multiple stuff will appear” either callbacks or eventemitters are generally be used (or even iterators)

I must admit that I went for Promise everywhere because they are extremely easy to "bind to" from the Rust side.

@jacogr
Copy link

jacogr commented Apr 20, 2019

100% - yes, came though quickly ;)

Understand the Promises interface, just probably need something where data is streamed - the callback approach may work best there.

However, will look in detail and stop popping in and out ;)

@amaury1093
Copy link

I quite like the fact that listen_on returns a Promise that returns an event object that contains a Promise to the next event

While this works, it's quite rare to see this pattern. A more idiomatic would be to use iterators (very similar): listen_on(addr) -> Iterator. Iterator is basically an object containing a .next() method, and this method returns a Promise<T>, where T here is an object { value: {new_addrs, expired_addrs, new_connections}, done: bool }

    const iterator = listen_on(addr);
    iterator.next().then(console.log); // { value: {new_addrs1, expired_addrs1, new_connections1}, done: false }
    iterator.next().then(console.log); // { value: {new_addrs2, expired_addrs2, new_connections2}, done: false }

An alternative (and actually more common way in JS, because iterators are still relatively new) it to use callbacks as Jaco proposed: listen_on(addr, cb) -> (), where cb has the signature (err, ({new_addrs, expired_addrs, new_connections})) -> ()

In js it's quite easy to switch from one to the other, so imo any of these 2 patterns would work, so do the one that's the easiest for you.

read is guaranteed to be called again only after the last returned Promise has succeeded, and not before.

In js Promises are one-shots, so connection.read().then(console.log) will only log once. So here too, either Iterators or callbacks.

One remaining issue I have is that in dial I would like to differentiate a situation where we fail to dial because we can't parse the multiaddress, and a situation where we fail to dial because the remote is unreachable. In the first situation, the caller should try a different transport. However I don't know how to represent that in JavaScript in an idiomatic way.

The easiest way I guess is that the Promise rejects with an Error, which contains a message with an error code/codename/mesage to be parsed.

@tomaka
Copy link
Member Author

tomaka commented Apr 23, 2019

Would it make sense for write() to have an iterator that produces functions, and the function gets passed the data to write?
The iterator produces a function whenever the underlying stream is ready to write again.

@amaury1093
Copy link

write can have the signature you initially proposed write(data) -> Promise. The promise resolves with undefined when the job is done.

// scenario 1
const promise1 = connection.write(data1);
const promise2 = connection.write(data2); // promise1 hasn't finished yet, so promise2 will reject with an error "connection still writing"

// scenario 2
connection
  .write(data1)
  .then(() => connection.write(data2)) // Writing data2 when data1 has finished
  .then(() => /* do something else when data2 has been written */)

I don't think there's a need for iterators here

@tomaka
Copy link
Member Author

tomaka commented Apr 23, 2019

I think it would be a bit awkward though if write is a method but read is just a field containing an iterator, no?

@amaury1093
Copy link

Could read be () -> Iterator? just so that everything that "sounds like a method" is a method

@tomaka
Copy link
Member Author

tomaka commented Apr 23, 2019

Ready for review!

As for the error thing, I added a parse_multiaddr() method. If it returns an error, we assume that the address is not supported. Then dial and listen_on are passed the object that parse_multiaddr returned. If dial or listen_on return an error, we assume that there was some I/O error or that the remote was unreachable.

@tomaka
Copy link
Member Author

tomaka commented Apr 23, 2019

Fwiw, here is the prototype I had to test my code: https://gist.github.com/tomaka/45265c26f93af0246c929116150359b7
It needs to be updated for the changes I just made.

@tomaka
Copy link
Member Author

tomaka commented Apr 23, 2019

As for the error thing, I added a parse_multiaddr() method. If it returns an error, we assume that the address is not supported. Then dial and listen_on are passed the object that parse_multiaddr returned. If dial or listen_on return an error, we assume that there was some I/O error or that the remote was unreachable.

I've just realized that this doesn't work. For WebSockets we support dialing but not listening. Trying to listen should indicate that we don't support the address (which makes libp2p try a different transport if there is one), and not that an error happened.

Any suggestion is welcome.

@tomaka
Copy link
Member Author

tomaka commented Apr 25, 2019

I'm now differentiate the errors based on error.name being equal to "NotSupportedError".

@tomaka tomaka merged commit 47a775d into libp2p:master Apr 25, 2019
@tomaka tomaka deleted the wasm-ext branch April 25, 2019 13:44
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wasm: add a transport that binds to a JS libp2p transport
4 participants