feat(pubsub): Add pubsub api #493

dignifiedquire · 2016-12-21T15:50:23Z

Rebased onto master and squashed #471

dignifiedquire · 2016-12-21T15:54:12Z

cc @diasdavid @haadcode

daviddias · 2016-12-21T16:02:51Z

Just tested it with new updates deps, tests pass in Node.js but seems to hang in the browser.

dignifiedquire · 2016-12-21T16:12:06Z

Just tested it with new updates deps, tests pass in Node.js but seems to hang in the browser.

Got them running again locally, but seeing some other errors, probably something got lost in the rebase, investigating

dignifiedquire · 2016-12-21T16:50:23Z

Fixed browser tests, in so far that they run through now. Looks like some of the pubsub tests are not passing yet though, probably due to timing issues with request abortion and unsubscribe calls.

dignifiedquire · 2016-12-21T18:35:43Z

The browser tests are failing because fetch requests can not be aborted. I tried switching to forced xhr, but they seem to be entirely failing and I don't understand why.

Before we ship this I would like to propose a change to this api, as I expect that this will cause more headaches down the line.

So the question is to what to change the subscribe api over http. I know of three ideas so far

Use FIN packets and leave the connection hanging or abort on the server side
Use websockets (ideally with heartbeat support + fin packet)
Use /unsubscribe to manually unsubscribe, and get the content from /topic?arg="topicname"

cc @diasdavid @haadcode @jbenet @whyrusleeping

whyrusleeping · 2016-12-21T18:53:04Z

It sounds like we just need to figure out how to get the xhr working, you can abort those.

dignifiedquire · 2016-12-21T18:57:58Z

xhr can not do streaming as far as I understand, meaning we will only get the response body once it's done. So making it effectively useless for us in this case, as you subscribe to get content

dignifiedquire · 2016-12-21T20:41:17Z

I've collected some interfaces from other places

RabbitMq: Uses AMQP which can be used over sockjs and websockets (https://www.rabbitmq.com/blog/category/web-messaging/)
Google Pubsub: https://cloud.google.com/pubsub/docs/reference/rest/v1/projects.subscriptions/pull uses a pulling mechanism
Firebase, seems to provide some sort of streaming over http requests: https://firebase.google.com/docs/reference/rest/database/#section-streaming

daviddias · 2016-12-22T08:07:01Z

It seems that..

2017 is the year of the new HTTP(+WebSockets) API 🎆 https://github.com/ipfs/http-api-spec/issues/116

daviddias · 2017-01-12T09:52:16Z

What are people thoughts here? Wanna consider disabling browser tests until http-api is updated, enabling people to use pubsub from Node.js?

daviddias · 2017-01-17T18:09:52Z

Update: ipfs/kubo#3522 (comment)

daviddias · 2017-01-17T18:50:07Z

Just tested with all the assertions possible and with regards to pubsub, it is 👌🏾

The only thing failing now, is name.resolve, but that seems like a new issue:

  1) .name .name.resolve:

      Uncaught AssertionError: expected { Object (Path) } to deeply equal { Object (Path) }
      + expected - actual

       {
      -  "Path": "/ipfs/Qma4hjFTnCasJ8PVp3mZbZK5g2vGDT4LByLJ7m8ciyRFZP"
      +  "Path": "/ipfs//ipfs/Qma4hjFTnCasJ8PVp3mZbZK5g2vGDT4LByLJ7m8ciyRFZP"
       }

@whyrusleeping

haadcode · 2017-03-20T15:59:16Z

(that said, if anyone wants to get the work done asap, feel free to continue this PR before I get to it)

mitar · 2017-03-22T00:16:31Z

BTW, for browser, we should probably use EventStreams, a w3c standard. Wikipedia is also using it now: https://blog.wikimedia.org/2017/03/20/eventstreams/

haadcode · 2017-03-23T09:12:08Z

@diasdavid Addressed your CR feedback. Are we good to merge? :)

haadcode · 2017-03-23T09:17:17Z

CI not happy yet, fixing.

daviddias

LGTM. It needs a rebase from master and a git commit squashing

daviddias · 2017-03-23T09:17:03Z

package.json

    "gulp": "^3.9.1",
    "hapi": "^16.1.0",
-    "interface-ipfs-core": "~0.24.1",
+    "interface-ipfs-core": "github:ipfs/interface-ipfs-core#fix\/pubsub-tests",


This needs to point to the latest interface-ipfs-core

daviddias · 2017-03-23T09:20:56Z

test/pubsub-in-browser.spec.js

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


haadcode · 2017-03-23T12:51:49Z

Rebased and squashed.

haadcode · 2017-03-23T13:47:42Z

Discovered the need to use dirty-chai for expect. Updated and replaced the head.

daviddias

small request, otherwise LGTM

daviddias · 2017-03-23T14:43:41Z

src/pubsub-message-stream.js

+    } catch (e) {
+      // Not a valid pubsub message
+      // go-ipfs returns '{}' as the very first object atm, we skip that
+      return callback()


Has this been reported? //cc @whyrusleeping @Kubuxu

There was a lengthy discussion about this in ipfs/kubo#3304. (short: it's always been like that, go team was involved)

daviddias · 2017-03-23T14:44:04Z

src/pubsub-message-stream.js

+    let msg
+    try {
+      msg = PubsubMessage.deserialize(obj, 'base64')
+    } catch (e) {


Always use err for errors to make sure they are captured by the linter. Here, this error is being silenced.

Fixed and amended the latest commit.

haadcode · 2017-03-23T17:24:00Z

The pubsub load tests are failing on Travis: https://travis-ci.org/ipfs/js-ipfs-api/jobs/214309202 (all travis platforms). Have no idea why. Will need to look into it tomorrow.

haadcode · 2017-03-24T09:35:35Z

Update on this from IRC:

04:55 <+haad> daviddias: pubsub is not supposed to guarantee delivery-in-order for the pubsub messages, right?
04:56 <+haad> ie. message 2 can be received by another peer before message 1
04:56 <@dignifiedquire> haad: correct, there is no ordering to the messages
05:02 <+haad> ok great, then that's the problem with failing travis tests. the messages are all received (correctly) but sometimes they come in different order
              but we expect them to arrive in order in our interface test. daviddias dignifiedquire I'll change the interface test to only test if all messages
              are received and remove the expectation that they are in order, sounds good?
05:05 <@dignifiedquire> from my side yes I think that makes sense
05:07 <+haad> I'll wait for wild d-dash to appear to make sure we're all in agreement that's what we should do :)

Will catch up with @diasdavid on IRC once he's online and decide how we proceed.

daviddias · 2017-03-24T10:11:55Z

"And then d-hash showed up"

10:03 <@daviddias> haad: it is not guaranteed
10:04 <@daviddias> haad: although it is weird that in a 1-1 directly connection same host
10:04 <@daviddias> that they arrive out of order

I wonder if go-ipfs has a weird way of queue the http requests to the API which makes them actually switch order.

haadcode · 2017-03-24T13:41:48Z

After some discussion on IRC and more investigation, it seems the messages in the load test get out of order on go-ipfs side. See #ipfs-dev for the full log, but here are the important parts:

06:39 <+haad> keks[m]: ok, makes sense. yeah so the issue we're seeing is that sometimes the messages are out of order for the pubsub load test (which
              sends/receives 10k messages as fast as it can) and our current test expects them to be in order. so we're wondering if we can drop the expectation
              that they're always in order.
06:40 < keks[m]> imho that is the right way to go
06:40 <+haad> cool
06:40 <+haad> cc daviddias ^
06:40 <+haad> Kubuxu: any thoughts on the above discussion?
06:40 <+haad> or lgierth
06:42 <@Kubuxu> haad: are there multiple sending daemons?
06:42 <+haad> Kubuxu: one is sending, one is receiving
06:46 <@Kubuxu> haad: so the seqnumber is nanotime. Can you check if seqnumber is in order.
06:46 <@Kubuxu> this would allow me to narrow it down
06:48 <+haad> Kubuxu: haven't checked if the seq number is in order but I believe they're not. we send "message #n" as the payload/data for each message which use to determine if the messages are in order.
06:49 <@Kubuxu> I am asking about seqnumber as this will tell me whether  it is floodsub or go-ipfs code.
06:50 <+haad> btw. if it helps, on Travis the load test runs at ~1000ops/s whereas locally it's around 300ops/s (ops == messages send + received / duration), so
              it's passing the message about 3x faster than locally (where it never fails)
06:52 <+haad> Kubuxu: what I'm saying is I don't know what the seq numbers look like but the payloads say they're out of order. is there a difference to payload
              vs. seq number that it can behave in different ways?
06:53 <@Kubuxu> yes, the payload is established in go-ipfs, the seqnumber after go-ipfs calls `SendMessage` after that the floodsub code looks quite stable
                regarding oredring.
06:53 <@Kubuxu> but I am not 100% sure
06:54 <+haad> ok
06:55 <+haad> I'll try to check that but it'll take a while as I need to wait for Travis to run it

And once Travis had run the tests:

08:47 <+haad> Kubuxu: I can confirm that the seqno and payload are matching (ie. when our check for order from payload fails, the sequence numbers are also out
              of order and they are in order otherwise)
08:47 <+haad> Kubuxu: does that help you narrow it down?
08:47 <@Kubuxu> yeah, and it is quite interesting
08:48 <+haad> you can take a look at the output here if you want: https://travis-ci.org/ipfs/js-ipfs-api/jobs/214612281. the funny thing is, it's (I believe)
              always one message that arrives out of order, other 9999 arrive in order :)
08:50 <+haad> does that mean it's in floodsub (as per your comment in PM)?
09:11 <@Kubuxu> yeah, looks like it, either in floodsub code or receiving side of go-ipfs
09:11 <@Kubuxu> sends are sequential 100% if seqnumbers are also swapped
09:30 <+haad> Kubuxu: then the question is, is that working as-intended or something we ought to fix?
09:30 <@Kubuxu> not sure, you would have to ask why
09:30 <+haad> ok

So @whyrusleeping, what's your take on this?

whyrusleeping · 2017-03-27T16:02:50Z

Floodsub does not guarantee the order in which you receive messages. They can come out of order, or not come at all

daviddias · 2017-03-30T13:25:19Z

I guess we need to take that assumption from the test then. (Still weird that in such a short network with almost 0 latency, there is disordering..)

dignifiedquire added the in progress label Dec 21, 2016

dignifiedquire mentioned this pull request Dec 21, 2016

WIP: feat/pubsub #471

Closed

dignifiedquire assigned dignifiedquire and unassigned dignifiedquire Dec 21, 2016

daviddias mentioned this pull request Dec 21, 2016

Feature: PubSub 🌟 ipfs/js-ipfs#644

Merged

dignifiedquire assigned daviddias and dignifiedquire Dec 22, 2016

daviddias mentioned this pull request Jan 17, 2017

[http-api] Unable to decode pubsub message property from ipfs/kubo#3522

Closed

daviddias assigned haadcode and unassigned dignifiedquire and daviddias Jan 19, 2017

daviddias added ready and removed in progress labels Jan 19, 2017

daviddias assigned gavinmcdermott, daviddias and dignifiedquire and unassigned gavinmcdermott and haadcode Jan 19, 2017

daviddias suggested changes Mar 23, 2017

View reviewed changes

haadcode force-pushed the feat/floodsub-rebase branch 2 times, most recently from 94d8bbc to 75ab123 Compare March 23, 2017 12:51

haadcode force-pushed the feat/floodsub-rebase branch 3 times, most recently from a04b794 to 1793e70 Compare March 23, 2017 13:47

daviddias suggested changes Mar 23, 2017

View reviewed changes

haadcode force-pushed the feat/floodsub-rebase branch 2 times, most recently from a5abedd to c49539d Compare March 23, 2017 16:14

haadcode mentioned this pull request Mar 27, 2017

Input on go-ipfs pubsub issue whyrusleeping/todo#32

Closed

daviddias approved these changes Mar 30, 2017

View reviewed changes

haadcode and others added 2 commits March 30, 2017 14:49

feat(pubsub): Add pubsub api

2a7c1e6

chore: update deps

511bd76

daviddias force-pushed the feat/floodsub-rebase branch from c49539d to 511bd76 Compare March 30, 2017 14:04

daviddias merged commit d2eb925 into master Mar 30, 2017

daviddias deleted the feat/floodsub-rebase branch March 30, 2017 14:15

daviddias removed the ready label Mar 30, 2017

alanshaw mentioned this pull request Feb 23, 2018

Allows embedded IPFS node to be configured ipfs/ipfs-companion#395

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pubsub): Add pubsub api #493

feat(pubsub): Add pubsub api #493

dignifiedquire commented Dec 21, 2016

dignifiedquire commented Dec 21, 2016

daviddias commented Dec 21, 2016

dignifiedquire commented Dec 21, 2016

dignifiedquire commented Dec 21, 2016

dignifiedquire commented Dec 21, 2016

whyrusleeping commented Dec 21, 2016

dignifiedquire commented Dec 21, 2016

dignifiedquire commented Dec 21, 2016

daviddias commented Dec 22, 2016 •

edited

Loading

daviddias commented Jan 12, 2017

daviddias commented Jan 17, 2017

daviddias commented Jan 17, 2017

haadcode commented Mar 20, 2017

mitar commented Mar 22, 2017

haadcode commented Mar 23, 2017

haadcode commented Mar 23, 2017

daviddias left a comment

daviddias Mar 23, 2017

daviddias Mar 23, 2017

haadcode commented Mar 23, 2017

haadcode commented Mar 23, 2017

daviddias left a comment

daviddias Mar 23, 2017

haadcode Mar 23, 2017

daviddias Mar 23, 2017

haadcode Mar 23, 2017

haadcode commented Mar 23, 2017

haadcode commented Mar 24, 2017

daviddias commented Mar 24, 2017

haadcode commented Mar 24, 2017 •

edited

Loading

whyrusleeping commented Mar 27, 2017

daviddias commented Mar 30, 2017

feat(pubsub): Add pubsub api #493

feat(pubsub): Add pubsub api #493

Conversation

dignifiedquire commented Dec 21, 2016

dignifiedquire commented Dec 21, 2016

daviddias commented Dec 21, 2016

dignifiedquire commented Dec 21, 2016

dignifiedquire commented Dec 21, 2016

dignifiedquire commented Dec 21, 2016

whyrusleeping commented Dec 21, 2016

dignifiedquire commented Dec 21, 2016

dignifiedquire commented Dec 21, 2016

daviddias commented Dec 22, 2016 • edited Loading

2017 is the year of the new HTTP(+WebSockets) API 🎆 https://github.com/ipfs/http-api-spec/issues/116

daviddias commented Jan 12, 2017

daviddias commented Jan 17, 2017

daviddias commented Jan 17, 2017

haadcode commented Mar 20, 2017

mitar commented Mar 22, 2017

haadcode commented Mar 23, 2017

haadcode commented Mar 23, 2017

daviddias left a comment

Choose a reason for hiding this comment

daviddias Mar 23, 2017

Choose a reason for hiding this comment

daviddias Mar 23, 2017

Choose a reason for hiding this comment

haadcode commented Mar 23, 2017

haadcode commented Mar 23, 2017

daviddias left a comment

Choose a reason for hiding this comment

daviddias Mar 23, 2017

Choose a reason for hiding this comment

haadcode Mar 23, 2017

Choose a reason for hiding this comment

daviddias Mar 23, 2017

Choose a reason for hiding this comment

haadcode Mar 23, 2017

Choose a reason for hiding this comment

haadcode commented Mar 23, 2017

haadcode commented Mar 24, 2017

daviddias commented Mar 24, 2017

haadcode commented Mar 24, 2017 • edited Loading

whyrusleeping commented Mar 27, 2017

daviddias commented Mar 30, 2017

daviddias commented Dec 22, 2016 •

edited

Loading

haadcode commented Mar 24, 2017 •

edited

Loading