WIP: feat/pubsub \o/ #610

gavinmcdermott · 2016-11-18T23:47:28Z

Note: I discovered the commit guidelines after I'd committed some things and synced with upstream, so I apologize for the messy history here.

Here's an initial pass at the FloodSub implementation. There's a bit of conversation on this in various threads (this js-ipfs-api PR, and some other general API discussion). That said, I wanted to get this up for comments. Thanks in advance, folks.

cc: @diasdavid @dignifiedquire

Add initial floodsub implementation

haadcode · 2016-11-19T09:54:06Z

src/core/components/floodsub.js

+      }
+
+      let rs = new Readable()
+      rs.cancel = () => self._floodsub.subscribe(topic)


This should prolly be unsubscribe, right?

Yes @haadcode thank you.

Addressed 👍

haadcode · 2016-11-19T09:57:28Z

I think we agreed in the API discussion to go with .publish, .subscribe and .unsubscribe for the names for the commands. I haven't changed this in js-ipfs-api yet but it's on my todo list for today.

Would be good if the API in this PR would be changed to that too. @gavinmcdermott what do you think?

haadcode · 2016-11-19T10:00:25Z

src/cli/commands/floodsub/pub.js

+log.error = debug('cli:floodsub:error')
+
+module.exports = {
+  command: 'pub <topic> <message>',


I think we should use <data> here as we do in go-ipfs (https://github.com/ipfs/go-ipfs/blob/master/core/commands/pubsub.go#L198), just to make it clear that there isn't a specific type of message but rather one can send any data as the message. Not super settled on this, so take it as a proposal :)

Sorry if I missed that (and I do like the full name convention). If you're good with it, I'll make that change to sync up with what you folks already agreed on.

Addressed 👍

haadcode · 2016-11-19T10:01:51Z

src/cli/commands/floodsub/start.js

+log.error = debug('cli:floodsub:error')
+
+module.exports = {
+  command: 'start',


We should prolly not have a separate command to start the pubsub but rather it should start "automatically" either on ipfs instance start (in goOnline() perhaps) or when subscribed to the first topic.

I agree with not having a start command. In an initial version I had it initialize when a call to pub or sub was made. My further comments on this are below :)

Agreed, the floodsub routine should start when a daemon goes online.

haadcode · 2016-11-19T10:07:27Z

Thank you @gavinmcdermott! This looks great! Left a couple of small comments. One bigger one we should discuss is the separate start command. I'd prefer if we didn't have one, so that the user doesn't have to start it manually before being able to use it. The floodsub can be started either when the node goes online (in goOnline()) or when subscribed to a topic for the first time. Would love to hear your comment and thoughts on this. cc @diasdavid

gavinmcdermott · 2016-11-19T18:30:16Z

Thank you for the feedback and thoughts @haadcode - I really appreciate it!

I agree with not having a start command. In an initial version I had it initialize when a call to pub or sub was made. And I like the idea of having something in goOnline(); this is actually where I started. However, it was causing all kinds of issues in tests (I think this dealt with FloodSub's dialing of peers). Which led me to think about the future of swappable pubsub implementations, and that the js-ipfs core might not want to rely on how various pubsub nodes connect to their peers...but maybe that's not relevant right now.

So perhaps the best near term step, which you arrived at above, is to initialize FloodSub from a call to pub or sub in the actual js-ipfs module.

gavinmcdermott · 2016-11-20T07:22:55Z

Hey @haadcode: latest changes are up and I made updates based on all comments except the one regarding the start command. And as you mentioned, it'd be good to get some thoughts from @diasdavid before diving into changes. Thanks again for the review, the one-liner cancel/subscribe bug was a great catch!

daviddias

This is looking good! :D Super happy to see floodsub being added. Thank you @gavinmcdermott !

@haadcode does this mean that this PR #531 won't be needed anymore?

daviddias · 2016-11-21T10:24:04Z

src/cli/commands/floodsub/start.js

+log.error = debug('cli:floodsub:error')
+
+module.exports = {
+  command: 'start',


Agreed, the floodsub routine should start when a daemon goes online.

daviddias · 2016-11-21T10:24:31Z

src/cli/commands/floodsub/subscribe.js

+log.error = debug('cli:floodsub:error')
+
+module.exports = {
+  command: 'subscribe <topic>',


can we have an alias as sub?

@diasdavid: Is the intention for this to be aliased from the top level as: jsipfs sub <some topic> or from the module as: jsipfs floodsub sub <topic>?

daviddias · 2016-11-21T10:25:13Z

src/cli/commands/floodsub/unsubscribe.js

+log.error = debug('cli:floodsub:error')
+
+module.exports = {
+  command: 'unsubscribe <topic>',


can we have an alias as unsub?

daviddias · 2016-11-21T10:26:13Z

src/core/components/floodsub.js

+      }
+
+      self._floodsub = new FloodSub(self._libp2pNode)
+      return callback(null, self._floodsub)


I believe it doesn't have to return the floodsub instance, as it gets available in the self object

daviddias · 2016-11-21T10:26:43Z

src/core/components/floodsub.js

+
+    subscribe: promisify((topic, options, callback) => {
+      // TODO: Clarify with @diasdavid what to do with the `options.discover` param
+      // Ref: https://github.com/ipfs/js-ipfs-api/pull/377/files#diff-f0c61c06fd5dc36b6f760b7ea97b1862R50


I've answered somewhere, but just to keep track: Currently we do nothing as js-ipfs doesn't have the go-ipfs DHT

daviddias · 2016-11-21T10:29:12Z

src/core/components/floodsub.js

+          data: data.toString(),
+          topicIDs: [topic]
+        })
+      })


This might cause an error that if I call subscribe on the same topic twice, I'll start seeing the same message being emitted twice as well

Now handled

daviddias · 2016-11-21T10:30:40Z

src/core/components/floodsub.js

+      callback(null)
+    }),
+
+    unsubscribe: promisify((topic, callback) => {


If a topic is unsubscribed, all of the event listeners for those topics should be unregistered as well, otherwise this will cause an eventual memory leak

daviddias · 2016-11-21T10:31:49Z

src/http-api/resources/floodsub.js

+        stream._read = () => {}
+        stream._readableState = {}
+      }
+      return reply(stream)


I have a hunch that this won't work. This stream of objects needs to be converted to ndjson stream

@diasdavid - I discovered the pull-ndjson module you published; any examples of the ndjson module being used in the ecosystem?

daviddias · 2016-11-21T10:33:28Z

test/core/both/test-floodsub.js

+      })
+    })
+  })
+})


It would be awesome to have these tests as part of interface-ipfs-core, so that we can use the same tests for js-ipfs and js-ipfs-api, making sure that both implementations follow the same interface.

See https://github.com/ipfs/interface-ipfs-core/tree/master/src for examples in how to achieve this.

daviddias · 2016-11-21T10:33:57Z

test/utils/ipfs-exec.js

@@ -20,7 +20,7 @@ module.exports = (repoPath, opts) => {
  env.IPFS_PATH = repoPath

  const config = Object.assign({}, {
-    stipEof: true,
+    stripEof: true,


nice catch 👍 //cc @dignifiedquire

daviddias · 2016-11-21T10:35:16Z

Note: I discovered the commit guidelines after I'd committed some things and synced with upstream, so I apologize for the messy history here.

No problem, rebasing and rewording should be a jiffy :) Thank you for adopting it!

haadcode · 2016-11-21T14:23:02Z

It'd be great @gavinmcdermott if you could make it to the js-ipfs or libp2p call tonight and we could all check in on the various pubsub efforts. Totally fine if you can't make it, we can continue here and in various other related issues.

There's a lot going on and I would like to make sure we're all on the same page as to a) what we need to do next and b) who's doing what, so that we avoid duplicated work and we can unify our efforts.

gavinmcdermott · 2016-11-22T00:22:09Z

@haadcode - Sorry for the delayed reply here—and I agree that syncing up would be great! I emailed @diasdavid yesterday weekend to let him know my status—I'm off to the backcountry for the next week starting tomorrow. Today was a travel day for me, and I'll have a connection until we head out tomorrow.

Syncing

If a call can't be managed, how about connecting over email? Feel free to drop me a line at gavinmcdermott@gmail.com, I'll have connection for a short while here. Sorry for the poor timing!

Latest Comments

I'll work on those changes.

Question on Init

@diasdavid / @haadcode - I initially had FloodSub being initialized within goOnline()(everyone's intuition is aligned here). However, I was getting some port reuse errors when running the test suite (e.g.: EADDRINUSE) and I believe it has something to do with the test harness and how floodsub dials peers immediately. I spent a bit of time on this issue before just tabling it to make progress on the whole.

Any ideas on:

What this issue is (or if I'm on the right track with my statement above)
Possible resolutions / ideas

Regardless, it'll get sorted out - so thanks for any ideas!

haadcode · 2016-11-22T18:37:32Z

@gavinmcdermott have a great holiday in the backcountry! :) We'll sync properly when you get back.

Unfortunately I have no idea re. the issue you're having re. connections and libp2p, that's all @diasdavid.

I've been working on the tests and js-ipfs-api implementation for pubsub. The short is: there were couple of bugs that made it tricky but there's PRs being merged as we speak and I expect we'll have a solid working version of go-ipfs pubsub sometime this week. Along with that, the js-ipfs-api implementation is almost complete and can be found here https://github.com/haadcode/js-ipfs-api/blob/fix/pubsub/src/api/pubsub.js. What is missing is pubsub/topics command but otherwise it's done. It was fairly tricky with the Stream so I highly recommend to take a look how I ended up structuring the code and what's doing what. This might be different on js-ipfs side, though. I think you might be able to use the PubsubMessage class for encapsulating the messages coming from floodsub, but you might not (as the data types might be different between js-ipfs and go-ipfs), so take a look and see if there's anything useful for you.

The good news is that there's not a fairly good test suite for pubsub which I plan to PR o interface-core as @diasdavid proposed somewhere. It might be helpful for you @gavinmcdermott to try running js-ipfs code against the same tests: https://github.com/haadcode/js-ipfs-api/blob/fix/pubsub/test/ipfs-api/pubsub.spec.js even if they're not in interface-core yet.

haadcode · 2016-12-05T15:14:16Z

Tests are now in ipfs-inactive/interface-js-ipfs-core#101. I'll try to run them against your PR @gavinmcdermott and see what comes out.

haadcode · 2016-12-05T15:56:52Z

@gavinmcdermott branched off from your floodsub branch and added the interface-ipfs-core tests, can't quite get my head around to push/PR to your repo but here's the relevant code: 699bd25. Once you have that, you need to checkout this PR ipfs-inactive/interface-js-ipfs-core#101 and run npm link inside the repo as well as running npm link interface-ipfs-core in your js-ipfs repo. That should do it.

That said, the tests are NOT passing as the very first throw "Floosub not started" (expected). If I start floodsub in go-online, js-ipfs tests just fail silently and don't give me any feedback as to why :/ Hope this helps.

haadcode · 2016-12-05T16:36:29Z

Heads-up: pubsub.ls() and pubsub.peers() are not yet implemented in libp2p-floodsub (they're required to satisfy the api).

haadcode · 2016-12-05T16:39:26Z

@gavinmcdermott I did some quick hacks trying to get the tests work. Can run some of them now: 6efd705.

gavinmcdermott · 2016-12-05T16:52:05Z

Thanks @haad! I've been seeing your messages come through - Today is my last travel day as I head back to SF. I'll hop on to the call from a coffee shop, but plan on my being available this week to get things landed. Looking forward to it. Thanks for the updates in the interim and talk soon!

On Mon, Dec 5, 2016 at 9:39 AM Haad ***@***.***> wrote: @gavinmcdermott <https://github.com/gavinmcdermott> I did some quick hacks trying to get the tests work. Can run some of them now: 6efd705 <6efd705> . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#610 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASfggj87hHqagdXOyWhHcuIKZaeC6dDks5rFD4_gaJpZM4K3Cv2> .

-- Sent from Gmail Mobile

gavinmcdermott · 2016-12-07T06:12:54Z

Update for @haadcode

This evening I am in the middle of two work items primarily:

Addressing David's suggestions from his code review
Adding the missing ls and peers APIs

Here are the updated ecosystem packages I'm now building against:

interface-ipfs-core: ipfs/interface-ipfs-core#5c7df414a8f627f8adb50a52ef8d2b629381285f
ipfs-api: ipfs/js-ipfs-api#01044a1f59fb866e4e08b06aae4e74d968615931

I think these are the latest based on the updates today. One item I did not get around to fixing (yet) is the issue with Floodsub initializing in goOnline(). Since it was causing issues with the bitswap tests (among others), David suggested wrapping up the changes and specifically running the Floodsub tests in isolation. Any additional suggestions here are welcome too.

Goal for tomorrow

My goal tomorrow is to get the above commit up for your review and get us into the home stretch! As a heads up, I'll be heading down to meet with David on Thursday to work on things...so we can also use that day to knock out any outstanding issues that are left after the next 36 hours.

Thanks and please let me know if you have any thoughts on this.

haadcode · 2016-12-07T06:33:00Z

@gavinmcdermott Thanks for the update! I just got up and once I've finished my coffee, will take a look at what you and @diasdavid did last night and pick up what I can. Will keep you updated on progress.

daviddias · 2016-12-07T17:30:56Z

Awesome \o/ excited to get this in. I'm sure that after getting this merged, there will be need some interop testing of a orbit+go-ipfs and orbit+js-ipfs, but with this PR we get closer to that than ever :)

@gavinmcdermott I've invited you to the JavaScript team at the IPFS org a while ago:
, from now on, it is easier if development happens inside a branch of the main repo, so that everyone can tag along (avoid PR the PR).

I was going to do this, but since there is CR that still needs to be addressed here, I didn't want to loose it. Will you be around today, @gavinmcdermott ?

gavinmcdermott · 2016-12-07T17:45:14Z

@diasdavid: For some reason, I don't have any pending invites in my account. Would you mind cancelling and re-inviting?

Regarding your pending CR: I intend to wrap that up today, and will be working on it primarily. Aside from a brief hackathon I'm running today from 11-3 (Pacific), I'm focused on this. Feel free to ping me over email / IRC whenever and I'll be responsive as quickly as possible in the interim.

daviddias · 2016-12-07T17:53:13Z

@gavinmcdermott sounds good :) Good luck for the hackathon!

re: invite: I need to ask @whyrusleeping or @jbenet to do it, once I get the hang of them, I'll ask, meanwhile, if you open https://github.com/ipfs, it doesn't show to you?

gavinmcdermott · 2016-12-07T18:02:18Z

@diasdavid: Just opened the link and it was waiting for me - all good!

gavinmcdermott · 2016-12-07T18:18:16Z

@diasdavid - since the interface-ipfs-core files are expecting a .pubsub on the node as opposed to the .floodsub, I am also going ahead and changing the property name over to .pubsub, and simply leave the module we're using as floodsub.

E.g.: self._pubsub = new FloodSub(libp2pNode) and node.pubsub...

I'll also change over the directory / test names as well.

daviddias · 2016-12-07T18:32:19Z

@gavinmcdermott yeah, what happened is that we reached consensus to make pubsub the interface and floodsub the implementation. I do still feel that this will create the compromise of having to change the pubsub interface several times, but it has the benefit of having users and developers about one single pubsub interface. Thank you :)

gavinmcdermott · 2016-12-08T06:18:17Z

Update for @haadcode and @diasdavid:

Today:

Updating the namespace from floodsub to pubsub (to fit with core tests, api, docs, etc...)
Addressed remaining, smaller issues from David's CR
Removed start and unsubscribe
Added the newer ls and peers api based on docs
- Includes the associated changes in the cli and http-api

Tonight:
Tests are now failing in several places with the latest changes. cli tests and core tests are what I'm working on tonight.

Tomorrow:
I will be working from Palo Alto with @diasdavid and folks. I feel like things will be in a place where we can talk through and wrap up any outstanding issues and get this PR ready to close. @haadcode, expect good things :)

haadcode · 2016-12-08T07:12:45Z

Sounds great! Looking forward to check this. I'll have time today to work on this, so perhaps there's a little more there when you wake up tomorrow :)

haadcode · 2016-12-08T07:13:18Z

And thank you for keeping everyone so well updated! It's crucial when working on the same thing as a team.

gavinmcdermott · 2016-12-08T08:19:24Z

@haadcode - The first batch of core interface tests are passing. I'm now running into an issue in the multiple nodes tests here. Basically the waitForPeers function continues to loop until the tests timeout because the nodes are disconnected, and as a result, the node.pubsub.peers is always empty.

I tried connecting the nodes manually in the before setup (see below) and a few other things, but before spending much more time, I wanted to see if you had any guidance...

(cb) => {
  ipfs1._libp2pNode.dialByPeerInfo(ipfs2._peerInfo, (err) => {
    expect(err).to.not.exist
    cb()
  })
}

Thank you in advance!

haadcode · 2016-12-08T10:09:41Z

The idea for those tests is to test that two peers can send and receive messages from/to each other in pubsub. pubsub.peers is supposed to return a list of peers who are connected to the same topic. If that doesn't return peers, then they're not considered connected.

How go-ipfs works is that it has the discovery flag which I guess makes it look for peers automatically. Since we can't do that in js-ipfs due to lacking DHT, we probably need to connect the two nodes manually in the tests. However, when I run js-ipfs instances, they usually get connected.

But perhaps we don't have discovery on js-ipfs, or whatever reason, the nodes don't seem to get connected, or at least the information that they're connected doesn't get all the way to pubsub.peers.

Does that make sense?

If you have network/connectivity problems, that's all @diasdavid and he'll be able to help out.

haadcode · 2016-12-08T10:10:04Z

@gavinmcdermott when you're done with your day, ping me here and I can pick it up and see if I can get further.

daviddias · 2016-12-08T20:46:22Z

I'm going to merge this branch of a fork into a branch of this repo -- feat/pubsub -- so that it is easier for us to be pushing to the same codebase (it will help a lot today that we are working from the same room).

gavinmcdermott added 5 commits November 11, 2016 21:18

fix cli test typo

d6c6d12

wip: floodsub now in api-routes, api-resources, and core-components

641fe1c

Initial core tests pass

d3c4f02

Merge with master

9026a88

feat(api, cli, core): add floodsub

9b7da97

Add initial floodsub implementation

jbenet added the status/deferred Conscious decision to pause or backlog label Nov 18, 2016

Merge branch 'master' into feature-floodsub

adeccec

haadcode mentioned this pull request Nov 19, 2016

track: floodsub #580

Closed

3 tasks

haadcode reviewed Nov 19, 2016

View reviewed changes

refactor(api, cli, core): update floodsub to a full name API

94bbc43

daviddias added the status/in-progress In progress label Nov 21, 2016

daviddias suggested changes Nov 21, 2016

View reviewed changes

daviddias changed the title ~~Merge feature-floodsub~~ add floodsub to js-ipfs \o/ Nov 21, 2016

daviddias added the enhancement label Nov 21, 2016

daviddias assigned haadcode Nov 27, 2016

chore: merge upstream/master

4ee7b5b

daviddias mentioned this pull request Dec 6, 2016

Feat/floodsub #531

Closed

daviddias changed the title ~~add floodsub to js-ipfs \o/~~ WIP: feat/pubsub \o/ Dec 6, 2016

gavinmcdermott added 2 commits December 8, 2016 12:28

chore: address CR comments

5031470

fix: fix lint issues

8084cb5

daviddias changed the base branch from master to feat/pubsub December 8, 2016 20:44

daviddias merged commit 3cdfcb0 into ipfs:feat/pubsub Dec 8, 2016

daviddias removed status/deferred Conscious decision to pause or backlog status/in-progress In progress labels Dec 8, 2016

daviddias mentioned this pull request Dec 8, 2016

Feature: PubSub 🌟 #644

Merged

daviddias mentioned this pull request Nov 11, 2017

Impl the http endpoints and cli cmds for PubSub #1068

Closed

WIP: feat/pubsub \o/ #610

WIP: feat/pubsub \o/ #610

Conversation

gavinmcdermott commented Nov 18, 2016 • edited by haadcode Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haadcode commented Nov 19, 2016

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

haadcode commented Nov 19, 2016

gavinmcdermott commented Nov 19, 2016

gavinmcdermott commented Nov 20, 2016

daviddias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavinmcdermott Dec 7, 2016 • 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

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

gavinmcdermott Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Nov 21, 2016

haadcode commented Nov 21, 2016

gavinmcdermott commented Nov 22, 2016

Syncing

Latest Comments

Question on Init

haadcode commented Nov 22, 2016

haadcode commented Dec 5, 2016

haadcode commented Dec 5, 2016

haadcode commented Dec 5, 2016

haadcode commented Dec 5, 2016

gavinmcdermott commented Dec 5, 2016 via email

gavinmcdermott commented Dec 7, 2016

Update for @haadcode

Goal for tomorrow

haadcode commented Dec 7, 2016

daviddias commented Dec 7, 2016

gavinmcdermott commented Dec 7, 2016

daviddias commented Dec 7, 2016 • edited Loading

gavinmcdermott commented Dec 7, 2016

gavinmcdermott commented Dec 7, 2016 • edited Loading

daviddias commented Dec 7, 2016

gavinmcdermott commented Dec 8, 2016 • edited Loading

haadcode commented Dec 8, 2016

haadcode commented Dec 8, 2016

gavinmcdermott commented Dec 8, 2016

haadcode commented Dec 8, 2016

haadcode commented Dec 8, 2016

daviddias commented Dec 8, 2016

gavinmcdermott commented Nov 18, 2016 •

edited by haadcode

Loading

gavinmcdermott Dec 7, 2016 •

edited

Loading

gavinmcdermott Dec 8, 2016 •

edited

Loading

daviddias commented Dec 7, 2016 •

edited

Loading

gavinmcdermott commented Dec 7, 2016 •

edited

Loading

gavinmcdermott commented Dec 8, 2016 •

edited

Loading