Various floodsub issues #51

richardschneider · 2017-11-19T06:09:54Z

Some of the issues were introduced by me in #49. Basically, the field topicCIDs is used in RPC messages and topicIDs in API messages.

See issue #50

Summary of changes

multiple peer connections
RPC message changed to have binary from
Avoid race conditions, by quietly ignoring unsub when shutdown

This reverts commit b8f66cd.

daviddias · 2017-11-19T06:36:06Z

src/peer.js

@@ -155,9 +155,6 @@ class Peer {
   * @returns {undefined}
   */
  close (callback) {
-    if (!this.conn || !this.stream) {
-      // no connection to close
-    }


Why remove?

Because it never invoked the callback, read the rest of the code.

This is still WIP, reviews are fine but DO NOT merge

I take it back. This code does nothing.

richardschneider · 2017-11-19T07:27:36Z

@whyrusleeping IMHO libp2p/go-libp2p-pubsub@25ef943#diff-9b6fbbdd6fa2676b0d87d60d96f4b556 does not help JSON marshalling. Now RPC messages are different than API messages (from is now bytes in RPC and B58 to the API)

Also, doesn't this break old go nodes that are expected a string.

But its way to lateto change this. Will just have to cope with.

whyrusleeping · 2017-11-19T18:23:22Z

@richardschneider bytes and string are the same thing from a protobuf perspective. Old go nodes arent a problem because we check api versioning.

In any case, the api json is meant to be human readable (for the most part). The random mess of binary data that we had before was unusable, which is why it got changed.

richardschneider · 2017-11-19T22:22:50Z

@diasdavid when a connection is closed, then the peer is removed from peers; which is reasonable.

However, if a peer has multiple connections, then we loose all knowledge of it. Basically, floodsub is designed to handle 1 connection per peer, not multiple connections per peer.

Sounds academic, but in the real world (js-go interop testing) I see two connections being made. And when one of the connections is closed, we forgot all about the peer.

floodsub needs to know about all connections and only remove the peer when the last connection is closed.

This implements a refernce counting scheme. See #51 (comment) One test is still failing.

richardschneider · 2017-11-20T07:38:32Z

@diasdavid finally its ready for review

daviddias · 2017-11-20T08:33:16Z

Sounds academic, but in the real world (js-go interop testing) I see two connections being made. And when one of the connections is closed, we forgot all about the peer.

Two Connections are made (note, two connections in the sense of libp2p, which for PubSub, are two muxed streams), one for outgoing and another for incoming (peerA -> peerB and peerB -> peerA)

floodsub needs to know about all connections and only remove the peer when the last connection is closed.

Agree, it should just remove the information about a Peer when everything is shutdown. The current logic was to just ignore that it has a link with another Peer if we do not intend to send anything else (no outgoing connection), it should not try to use the incoming connection.

I'm reviewing this PR, just wanted to clarify this first :)

daviddias · 2017-11-20T08:37:28Z

src/index.js

+      log('new peer', id)
+      this.peers.set(id, peer)
+      existing = peer
+    }


This if can be inverted for great readability (by this I mean, avoid do negative equals with things like undefined or null)

let existing = this.peers.get(id) if (!existing) { log('new peer', id) this.peers.set(id, peer) existing = peer } else { log('already existing peer', id) ++existing._references }

Good point, positive logic is always better.

even better

if (existing) { log('already existing peer', id) ++existing._references } else { log('new peer', id) this.peers.set(id, peer) existing = peer }

daviddias · 2017-11-20T08:38:54Z

src/index.js

-    // If already had a dial to me, just add the conn
-    if (!this.peers.has(idB58Str)) {
-      this.peers.set(idB58Str, new Peer(peerInfo))
-    }


This is important logic.

What is happening here is: "If the other peer has already dialed to me, we already have an establish link between the two, what might be missing is a Connection specifically between me and that Peer"

This was copy and paste from original code. will add the comment.

The code was moved to _addPeer

daviddias · 2017-11-20T08:39:33Z

test/2-nodes.js

@@ -287,7 +287,7 @@ describe('basics between 2 nodes', () => {
      ], done)
    })

-    it('peer is removed from the state when connection ends', (done) => {
+    it.skip('peer is removed from the state when connection ends', (done) => {


Please do not skip tests.

I can't get it to work. Please tell me what to do. It's the whole multiple connections issue.

Seems that what we need is to track the Outgoing Connection and the Incoming Connection. This test should check that our Outgoing Connection disappears when the connection ends.

@diasdavid Here's an approach

it('peer is removed from the state when connection ends', (done) => { nodeA.dial(nodeB.peerInfo, (err) => { expect(err).to.not.exist() setTimeout(() => { expect(first(fsA.peers)._references).to.equal(2) expect(first(fsB.peers)._references).to.equal(2) fsA.stop(() => setTimeout(() => { expect(first(fsB.peers)._references).to.equal(1) done() }, 250)) }, 1000) }) })

daviddias · 2017-11-20T08:39:44Z

test/utils.spec.js

+      { from: binaryId }
+    ]
+    expect(utils.normalizeOutRpcMessages(m)).to.deep.eql(expected)
+  })


richardschneider · 2017-11-20T09:28:38Z

@diasdavid requested changes made.

What do you want to do about the skipped test.

…ion event

richardschneider · 2017-11-22T03:56:51Z

src/index.js

+        return peer.sendSubscriptions(topics)
+      }
+      const onConnection = () => {
+        peer.removeListened('connection', onConnection)


@pgte Should this be removeListener

@richardschneider oops, thanks! @diasdavid fixed it for me..

richardschneider added 5 commits November 18, 2017 17:46

fix: startup

2835336

Revert "fix: Published message field names (#49)"

6a45bfe

This reverts commit b8f66cd.

fix: close should always invoke callback

e8df6d6

fix: avoid race conditions, by quietly ignoring unsub when shutdown

81d50fe

fix: only delete peer when connections match

9eb14e7

ghost assigned richardschneider Nov 19, 2017

ghost added the status/in-progress In progress label Nov 19, 2017

fix: lint errors

8303a97

daviddias reviewed Nov 19, 2017

View reviewed changes

richardschneider added 3 commits November 20, 2017 08:19

fix: more work on connection shutdown

3edacfe

fix: RPC msg.from is now binary

ff77021

fix: lint errors

eaedffe

richardschneider added 4 commits November 20, 2017 16:10

fix: multiple connections to/from same peer

21773d0

This implements a refernce counting scheme. See #51 (comment) One test is still failing.

fix: topicCIDs => topicIDs

19adfd7

test: can not get this test to work!

2246baf

fix: lint errors

0e3af5e

daviddias suggested changes Nov 20, 2017

View reviewed changes

daviddias mentioned this pull request Nov 20, 2017

PubSub Interop Tests and CLI+HTTP-API Implementation ipfs/js-ipfs#1081

Merged

5 tasks

richardschneider added 2 commits November 20, 2017 22:05

fix: review changes

bfeb6ae

fix: lint errors on a comment, this going too far

6aa60ef

richardschneider added 2 commits November 21, 2017 16:50

test: get test working

35a94aa

test: add tests for publishing an array of messages

2e8dbe6

daviddias approved these changes Nov 21, 2017

View reviewed changes

subscribe: polling for connectivity replaced by handling peer connect…

9537fb7

…ion event

ghost assigned pgte Nov 21, 2017

richardschneider commented Nov 22, 2017

View reviewed changes

fix typo

0dfd20a

ghost assigned daviddias Nov 22, 2017

daviddias merged commit 45c9b11 into master Nov 22, 2017

daviddias deleted the issue-50 branch November 22, 2017 09:49

ghost removed the status/in-progress In progress label Nov 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various floodsub issues #51

Various floodsub issues #51

richardschneider commented Nov 19, 2017 •

edited

daviddias Nov 19, 2017

richardschneider Nov 19, 2017

richardschneider Nov 19, 2017

richardschneider commented Nov 19, 2017 •

edited

whyrusleeping commented Nov 19, 2017

richardschneider commented Nov 19, 2017

richardschneider commented Nov 20, 2017

daviddias commented Nov 20, 2017

daviddias Nov 20, 2017

richardschneider Nov 20, 2017

richardschneider Nov 20, 2017 •

edited

daviddias Nov 20, 2017

richardschneider Nov 20, 2017

richardschneider Nov 20, 2017

daviddias Nov 20, 2017

richardschneider Nov 20, 2017

daviddias Nov 20, 2017

richardschneider Nov 21, 2017

daviddias Nov 20, 2017

richardschneider Nov 20, 2017

richardschneider commented Nov 20, 2017

richardschneider Nov 22, 2017

pgte Nov 22, 2017

Various floodsub issues #51

Various floodsub issues #51

Conversation

richardschneider commented Nov 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardschneider commented Nov 19, 2017 • edited

whyrusleeping commented Nov 19, 2017

richardschneider commented Nov 19, 2017

richardschneider commented Nov 20, 2017

daviddias commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardschneider Nov 20, 2017 • edited

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

richardschneider commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardschneider commented Nov 19, 2017 •

edited

richardschneider commented Nov 19, 2017 •

edited

richardschneider Nov 20, 2017 •

edited