Skip to content
This repository has been archived by the owner on Feb 24, 2021. It is now read-only.

Async endeavour: Use promises #108

Merged
merged 43 commits into from
Nov 22, 2019
Merged

Async endeavour: Use promises #108

merged 43 commits into from
Nov 22, 2019

Conversation

mkg20001
Copy link
Member

@mkg20001 mkg20001 commented Jul 18, 2019

I've implemented secio in an async way

For now only the main methods have been done, as I'm unsure whether or not the approach taken by me fits into the libp2p ecosystem I'm refactoring the whole thing, yet it would be great if somebody could review it during the refactor, as I'm unsure whether or not the approach taken by me fits into the libp2p ecosystem

Basically: The callback(s) have been completely replace by a new variable encryptedConnection.awaitConnected that can be awaited to see if any errors occured

@mkg20001
Copy link
Member Author

Seems to mysteriously hang at reading proposal, sometimes
screenshot

@mkg20001
Copy link
Member Author

Ref ipfs/js-ipfs#1670

@mkg20001
Copy link
Member Author

Seems like it sometimes sets remoteId to undefined and then there's uncaught crash in src/index.js where it tries to create a peerinfo from the peerid that is undefined

@mkg20001
Copy link
Member Author

With the new interface-connection being finished soon, should I first just port secio to async/await or already implement the async iterators?

@jacobheun
Copy link
Contributor

@mkg20001 async iterators. I'm working on the internal refactor for the switch around the crypto upgrade. I am implementing the plaintext 2.0.0 spec in async iterators as part of that to make sure the upgrade process works smoothly, as well as sorting out any bugs with the current async updates.

@mkg20001
Copy link
Member Author

@jacobheun Could you give me some guidance on how to implement them?

Btw, it seems like I can't push to this branch

@jacobheun
Copy link
Contributor

@mkg20001 yes, once I get the initial dial flow working and any bugs worked out I am aiming to create a contributor guide for the async iterators to help us migrate the remaining modules. Hoping to get started on that in the next couple days.

@jacobheun
Copy link
Contributor

Btw, it seems like I can't push to this branch

This should be fixed, looks like the teams got shuffled a bit.

.aegir.js Outdated Show resolved Hide resolved
@jacobheun
Copy link
Contributor

@mkg20001 I opened a PR for a crypto interface for the async work, libp2p/js-libp2p-interfaces#2, which also includes a crypto mock in the test suite for those tests which shows how the crypto streams might get wrapped internally. I'm going to be building plaintext/2.0.0 off of that. I also added an example and some resources to the streaming iterables guide in js-libp2p, libp2p/js-libp2p#466.

@jacobheun
Copy link
Contributor

@mkg20001 it-length-prefixed 3.0.0 is out now, https://github.com/alanshaw/it-length-prefixed. It includes lp.decode.fromReader (was decodeFromReader in my branch), and support for the custom length coder which is included there for 32intBE.

@mkg20001
Copy link
Member Author

@jacobheun I've updated it-pb-rpc and added the LP encoder/decoder. I'll fix the rest of the mentioned comments and then this should be ready! 🎉

src/handshake/finish.js Outdated Show resolved Hide resolved
@jacobheun
Copy link
Contributor

There was an issue with benchmarks that was causing the performance to appear to be much higher than it actually is. benchmark doesn't support promises so the deferred still needs to be called. I fixed the issue and the results are actually similar to benchmarks against master, there's not a significant difference.

I am looking into an issue with compatibility against the current version of libp2p. I believe there are a couple places we are still using varint that is causing the issue. I'm looking to fix that today so we can get this merged.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@jacobheun
Copy link
Contributor

I created a PR in mkg20001/it-pb-rpc#2 to add support for passing the fixed encoder. The last issue with compatibility I am working on is that this branch is currently just sending the final handshake of nonces over the unencrypted connection. The nonces have to be sent over the newly constructed crypto box streams so that the encryption can be validated.

@mkg20001
Copy link
Member Author

The nonces have to be sent over the newly constructed crypto box streams so that the encryption can be validated.

Aren't they currently?

@jacobheun
Copy link
Contributor

Aren't they currently?

No, the write and read in finish are being done before the box/unbox pipe is created. It's easiest to see it in the network traffic, only the 16 bytes of the nonce are exchanged.

I pushed up 840c217 which fixes it. I am working on adding a test that will verify the exchange is compliant with the spec.

@jacobheun
Copy link
Contributor

I've added the tests for inbound and outbound (which are the same since it's symmetric) encryption so we can verify the upgrade process. I also updated the dependency list and added node 12 to CI.

Here are the most recent benchmarks:
Async/await

create peers for test x 653 ops/sec ±29.95% (68 runs sampled)
establish an encrypted channel x 207 ops/sec ±1.65% (77 runs sampled)
send plaintext 10 x 262144 bytes x 5,226 ops/sec ±2.34% (71 runs sampled)
send encrypted 10 x 262144 bytes x 41.79 ops/sec ±1.18% (65 runs sampled)
send plaintext 100 x 262144 bytes x 547 ops/sec ±3.42% (26 runs sampled)
send encrypted 100 x 262144 bytes x 5.21 ops/sec ±0.93% (29 runs sampled)
send plaintext 1000 x 262144 bytes x 52.61 ops/sec ±2.70% (59 runs sampled)
send encrypted 1000 x 262144 bytes x 0.55 ops/sec ±0.50% (7 runs sampled)

master

create peers for test x 646 ops/sec ±28.13% (70 runs sampled)
establish an encrypted channel x 192 ops/sec ±1.52% (78 runs sampled)
send plaintext 10 x 262144 bytes x 4,135 ops/sec ±2.82% (62 runs sampled)
send encrypted 10 x 262144 bytes x 38.24 ops/sec ±1.09% (62 runs sampled)
send plaintext 100 x 262144 bytes x 521 ops/sec ±2.19% (70 runs sampled)
send encrypted 100 x 262144 bytes x 4.91 ops/sec ±0.78% (28 runs sampled)
send plaintext 1000 x 262144 bytes x 53.99 ops/sec ±3.25% (61 runs sampled)
send encrypted 1000 x 262144 bytes x 0.51 ops/sec ±1.49% (7 runs sampled)

I have also run a test with an echo protocol of libp2p@latest and libp2p@refactor/async over tcp with mplex and secio. Dials and echos are working both ways, this should be good to go!

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jacobheun jacobheun merged commit 8ad4c15 into master Nov 22, 2019
@jacobheun jacobheun deleted the feat/promises branch November 22, 2019 20:18
@jacobheun
Copy link
Contributor

We resolved npm perms issues, the beta version 0.12.x is now on npm. @mkg20001 thank you for all your work on this!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants