refactor: async it #153

vasco-santos · 2019-11-20T20:53:21Z

This PR aims to use async iterators instead of pull-streams in the context of the asyn migration, as a follow up for #148

vasco-santos · 2019-11-21T21:17:43Z

package.json

-    "libp2p-mplex": "~0.8.5",
-    "libp2p-switch": "~0.42.7",
-    "libp2p-tcp": "~0.13.0",
+    "it-pair": "^1.0.0",


I will create an issue to track the removal of peer-book for tests, as well as handle the peer-store operations currently done here.

async can be removed from the dependencies now. And chai-checkmark needs to be a dev dep.

we still use async/queue for the query WorkerQueue. I will create an issue to track that

vasco-santos · 2019-11-21T21:26:46Z

test/multiple-nodes.spec.js

@@ -100,10 +96,10 @@ describe('multiple nodes', () => {
    await dhts[4].put(key, Buffer.from('world4'))

    const res = await Promise.all([
-      dhts[3].get(key, { maxTimeout: 2000 }),


I dont't know as this was passing before.

It was getting stored locally world3 (after put), which was the first result obtained from getMany. Then, we use the selector: () => 0 and the first record was obtained

dirkmc

Looks good! Just left a couple of nits and suggestions

src/index.js

src/network.js

dirkmc · 2019-11-22T14:22:40Z

src/network.js

+    ),
+    lp.decode(),
+    async source => {
+      for await (const chunk of source) {


Not sure if it's worth adding a dependency just for this, but the streaming iterables library has some useful functions, eg this could be replaced with a map and a collect

dirkmc · 2019-11-22T14:23:13Z

src/rpc/index.js

   * @param {PeerInfo} peer
   * @param {Message} msg
   * @returns {Promise<Message>}
   *
   * @private
   */
-  async function handleMessage (peer, msg) {
+  async function handleMessage (peer, msg) { // eslint-disable-line
+    // get handler & exectue it


Suggested change

// get handler & exectue it

// get handler & execute it

src/rpc/index.js

dirkmc · 2019-11-22T14:27:37Z

src/utils.js

@@ -11,6 +11,16 @@ const { Record } = require('libp2p-record')
 const PeerId = require('peer-id')
 const errcode = require('err-code')

+exports.itFilter = function (predicate) {


Consider using https://github.com/bustle/streaming-iterables#filter

jacobheun · 2019-11-22T17:19:16Z

package.json

-    "libp2p-mplex": "~0.8.5",
-    "libp2p-switch": "~0.42.7",
-    "libp2p-tcp": "~0.13.0",
+    "it-pair": "^1.0.0",


async can be removed from the dependencies now. And chai-checkmark needs to be a dev dep.

src/network.js

jacobheun · 2019-11-22T17:40:04Z

src/network.js

-    await this.dht._add(peer)
-    this._log('added to the routing table: %s', peer.id.toB58String())
+    // Open a stream with the connected peer
+    await conn.newStream(c.PROTOCOL_DHT)


This is unnecessary now, correct? We previously dialed the protocol to check support, but we already have that information now. We're not performing an explicit action, so we shouldn't create a stream.

jacobheun · 2019-11-22T17:44:02Z

src/network.js

@@ -151,10 +154,10 @@ class Network {
   *
   * @param {Connection} conn - the connection to use


There are a few places in the jsdocs that inaccurately reference Connection. We should make sure these are duplex iterables.

jacobheun · 2019-11-22T18:13:13Z

test/query/index.spec.js

          _peerInfo: ourPeerInfo,
-          _peerBook: new PeerBook()
-        }
+          _peerBook: peerStore


dialer._peerBook and dialer._peerInfo should be deleted, they're not properties of the dialer.

jacobheun · 2019-11-22T18:14:21Z

test/rpc/handlers/get-value.spec.js

@@ -87,6 +87,7 @@ describe('rpc - handlers - GetValue', () => {

      const msg = new Message(T, key, 0)

+      dht.peerStore.put(other)


Did this need to get added?

Yes, in the code we try to find the peer in the peer-store. After this test we have another one for unkwown peers

jacobheun · 2019-11-22T18:26:54Z

test/rpc/index.spec.js

+          data.push(chunk.slice())
+        }
+      }
+    )


If you use collect from streaming-iterables I think this code can be simplified a bit (as well as some of the other places we are doing collects here. Also, there are multiple slices of the BufferList happening in this stream, it's not needed. You could also use the it-buffer module and simplify all of this to be:

const data = await pipe( [msg.serialize()], lp.encode(), collect ) const duplexStream = { source: data, sink: async (source) => { const res = await pipe( source, lp.decode(), toBuffer, // Ensure we have buffers here for validateMessage to consume collect ) validateMessage(res) } }

yap, cool tip!

test/utils/index.js

Co-Authored-By: dirkmc <dirkmdev@gmail.com> Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

jacobheun

looks good! 🚢 🚀

vasco-santos force-pushed the refactor/async-iterators branch 13 times, most recently from efece9f to 0fc408a Compare November 21, 2019 20:08

refactor: async it

89f85bf

vasco-santos force-pushed the refactor/async-iterators branch from 0fc408a to 89f85bf Compare November 21, 2019 21:27

vasco-santos commented Nov 21, 2019

View reviewed changes

vasco-santos marked this pull request as ready for review November 21, 2019 21:28

vasco-santos requested review from dirkmc and jacobheun November 21, 2019 21:29

vasco-santos mentioned this pull request Nov 21, 2019

refactor: dht libp2p/js-libp2p#480

Merged

1 task

dirkmc approved these changes Nov 22, 2019

View reviewed changes

jacobheun suggested changes Nov 22, 2019

View reviewed changes

vasco-santos force-pushed the refactor/async-iterators branch 2 times, most recently from a10506e to 5ad4af7 Compare November 22, 2019 21:27

chore: apply suggestions from code review

44054b2

Co-Authored-By: dirkmc <dirkmdev@gmail.com> Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

vasco-santos force-pushed the refactor/async-iterators branch from 5ad4af7 to 44054b2 Compare November 22, 2019 21:40

vasco-santos requested a review from jacobheun November 22, 2019 22:31

fix: use libp2p dialer

a0a8500

vasco-santos force-pushed the refactor/async-iterators branch from 9ca9a2b to a0a8500 Compare November 22, 2019 22:38

alanshaw mentioned this pull request Nov 25, 2019

Awesome Endeavour: Async Iterators ipfs/js-ipfs#1670

Closed

jacobheun approved these changes Nov 26, 2019

View reviewed changes

vasco-santos merged commit dc824d9 into master Nov 26, 2019

vasco-santos deleted the refactor/async-iterators branch November 26, 2019 12:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: async it #153

refactor: async it #153

vasco-santos commented Nov 20, 2019 •

edited

Loading

vasco-santos Nov 21, 2019

jacobheun Nov 22, 2019

vasco-santos Nov 22, 2019

vasco-santos Nov 21, 2019

dirkmc left a comment

dirkmc Nov 22, 2019

dirkmc Nov 22, 2019

dirkmc Nov 22, 2019

jacobheun Nov 22, 2019

jacobheun Nov 22, 2019

jacobheun Nov 22, 2019

jacobheun Nov 22, 2019

jacobheun Nov 22, 2019

vasco-santos Nov 22, 2019

jacobheun Nov 22, 2019

vasco-santos Nov 22, 2019

jacobheun left a comment

		@@ -151,10 +154,10 @@ class Network {
		*
		* @param {Connection} conn - the connection to use

		@@ -87,6 +87,7 @@ describe('rpc - handlers - GetValue', () => {

		const msg = new Message(T, key, 0)

		dht.peerStore.put(other)

refactor: async it #153

refactor: async it #153

Conversation

vasco-santos commented Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirkmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobheun left a comment

Choose a reason for hiding this comment

vasco-santos commented Nov 20, 2019 •

edited

Loading