Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: compatibility with go-libp2p-mdns #80

Merged
merged 17 commits into from May 9, 2019

Conversation

4 participants
@alanshaw
Copy link
Member

commented Apr 8, 2019

This PR adds a compatibility class that allows a js-libp2p node to find a go-libp2p node (and vice versa) over MDNS. I do not know when go-libp2p plans to land the long awaited new MDNS implementation but until then this will allow interop between js and go.

It's implemented as a separate class so the two differing implementations do not get confused.

Fun fact, it uses 2 dgram servers. 1 is the multicast listener, which simply responds to queries. The other is a dgram server that exists for 5 seconds at a time, sends a multicast query, waits for unicast responses, stops and then starts up again on a different random port.

I've verified this is working correctly by running a go-ipfs and js-ipfs node with no boostrap nodes (and no other discovery methods) and verifying they find each other.

TODO:

  • Add tests!

Screenshot 2019-04-15 at 16 32 49


Some tips if you want to try this out:

  • After you've run ipfs init, remember to remove the bootstrap nodes from the config file (~/.ipfs/config) of each node before you start up
  • Use ipfs log level mdns debug for some go-ipfs mdns logs
  • You can use the following script (after npm linking this branch) to start a js-ipfs node with no bootstrap nodes and no discovery modules other than MDNS:
const IPFS = require('ipfs')
const MDNS = require('libp2p-mdns')
const TCP = require('libp2p-tcp')

const ipfs = new IPFS({
  repo: '/tmp/ipfs-mdns',
  config: {
    Bootstrap: []
  },
  libp2p: {
    modules: {
      peerDiscovery: [MDNS],
      transport: [TCP]
    }
  }
})

ipfs.on('ready', async () => {
  console.log('ipfs is ready')
  console.log('My Peer ID:', (await ipfs.id()).id)
  setInterval(async () => {
    const peers = await ipfs.swarm.peers()
    console.log(peers.length, 'peers:')
    peers.forEach(p => console.log(p.peer.toB58String()))
  }, 10000)
})

@ghost ghost assigned alanshaw Apr 8, 2019

@ghost ghost added the in progress label Apr 8, 2019

@dirkmc
Copy link

left a comment

I left a couple of comments about event listeners, otherwise looks good 👍

Would it make sense to branch of the async/await version of this package?

// Create a querier that queries multicast but gets responses unicast
const querier = MDNS({ multicast: false, interface: '0.0.0.0', port: 0 })

querier.on('response', this._onResponse)

This comment has been minimized.

Copy link
@dirkmc

dirkmc Apr 8, 2019

I think having this event listener will prevent the querier objects from being garbage collected
(looking at the source code for mdns, it doesn't seem like destroy() removes all event listeners)

}

stop (callback) {
this._mdns.destroy(callback)

This comment has been minimized.

Copy link
@dirkmc

dirkmc Apr 8, 2019

this._mdns.removeListener('query', this._onQuery)

@vasco-santos
Copy link
Member

left a comment

Great work Alan! 🥇

I think that it is a good call to have this in a separate class!

Regarding @dirkmc 's point for branching out from the async await branch, according to the table in ipfs/js-ipfs#1670 it is a P3+, and should have a lot of dependencies to get merged first. However, the async migration PR will have to be updated with this. Looking at the code, it does not seem a lot of work. What we can also do is changing this PR's codebase (compat) to use async await from the beginning, which would ease the other PR to get merged afterward. What do you guys think?

Also, do not forget to provide documentation for the new classes added.

return { stop: callback => querier.destroy(callback) }
}, this._options.queryPeriod)

setImmediate(() => callback())

This comment has been minimized.

Copy link
@vasco-santos

vasco-santos Apr 9, 2019

Member

Use process.nextTick instead

if (this._goMdns) {
this._goMdns.start(callback)
} else {
setImmediate(() => callback())

This comment has been minimized.

Copy link
@vasco-santos

vasco-santos Apr 9, 2019

Member

process.nextTick

start (callback) {
this._mdns = MDNS()
this._mdns.on('query', this._onQuery)
setImmediate(() => callback())

This comment has been minimized.

Copy link
@vasco-santos

vasco-santos Apr 9, 2019

Member

process.nextTick

Show resolved Hide resolved src/compat/responder.js Outdated
data: peerServiceTagLocal
})

// Only announce TCP multiaddrs for now

This comment has been minimized.

Copy link
@vasco-santos

vasco-santos Apr 9, 2019

Member

Can you have as an action item of merging this PR to create an issue for tracking the announce of other transports?

@alanshaw

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

What we can also do is changing this PR's codebase (compat) to use async await from the beginning, which would ease the other PR to get merged afterward. What do you guys think?

I don't think we should mix the two styles in the code base:

IMHO we should only mix promises and callbacks when we have to deal with an external library we have no control over. We already have async as a dependency so we'd not be saving any bundle bytes by using builtins. I think it sends the wrong message to contributors. We have a awesome endeavour where we're trying to coordinate all of this at the same time and adding Promises here piecemeal is problematic for a number of reasons:

  1. It's super inconsistent with the rest of the libp2p code (everything is callbacks, not even a dual promise/callback api)
  2. Sets the precedent for other contributors to do the same thing, when actually we want contributions to either focus on fixing problems or focus on the endeavour
  3. Finally, without going into specifics, mixing callbacks and Promises is fraught with double callback danger, perhaps not in this specific case, but it can be super hard to debug when it happens!

from libp2p/js-libp2p-switch#310 (comment)

So either we merge as is or rebase off of the async/await branch.

@jacobheun

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

My main concern here relates to #71, in regards to polling mdns. Currently this is configured to run the go query every 5 seconds by default. The normal query is set to run every 10s by default. Running 3 queries every 10 seconds is abusive to mdns and devices using it.

I think this might be a good opportunity for us to look at disabling polling by default, and allowing an interval to be set, instead of defaulting to polling. As new devices join, we should discover them when they broadcast.

@alanshaw

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

My main concern here relates to #71, in regards to polling mdns. Currently this is configured to run the go query every 5 seconds by default. The normal query is set to run every 10s by default. Running 3 queries every 10 seconds is abusive to mdns and devices using it.

Ok, there are some options:

  1. We replace the current implementation with the go compatible implementation
    • (+) Smaller code base, compatible with go-ipfs
    • (-) We poll every 5 seconds (but we could just increase that to 10s so we're no worse than we are already)
    • (-) We break MDNS discovery of older js-ipfs nodes running on the network (although this is already broken for other reasons libp2p/js-libp2p-switch#326)
  2. We turn off polling (but not responding) for Go MDNS compat
    • (+) Not introducing more MDNS polling
    • (+) go-ipfs nodes still poll so we'll send responses and go-ipfs nodes will probably connect to us (i.e. go-ipfs can discover js-ipfs but not the other way round)
    • (-) If go-ipfs nodes stop polling they'll never discover js-ipfs nodes

I think this might be a good opportunity for us to look at disabling polling by default, and allowing an interval to be set, instead of defaulting to polling. As new devices join, we should discover them when they broadcast.

Simply sending a multicast go-ipfs compatible "response" on an interval might work because I think go-ipfs accepts responses multicast as well as unicast. It's similar to (2) above, except that go-ipfs nodes will discover us when we send a response in an interval, not as a response to their query.

I'll check it out and report back.

@alanshaw

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

It works! #81

@jacobheun

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

So, #81 doesn't do a query at all which is why we won't be able to get go nodes. It should still do a query, it just shouldn't need to be run on an interval.

Anytime a node starts, it should listen for MDNS queries and then immediately query.
That same query will be received by the started node, which will respond to it. So we've discovered any existing peers, and told them about ourselves.

If we start another node 10 minutes later and it does the same thing. The first node should be made aware of the new node, and vice versa.

For every node we have, we should only need that many queries, as long as everyone responds. There is the problem of sleeping nodes or network changes, but since our discovery doesn't work all that consistently right now, I think that could be addressed in a followup release or temporarily just increasing the interval.

@alanshaw

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

@jacobheun I've altered this PR to increase the interval between MDNS queries to 1 min.

When GoMulticastDNS starts it creates 2 MDNS servers:

  1. A Responder which listens for multicast queries and responds DIRECTLY to them i.e. a unicast response
  2. A Querier which listens on a RANDOM port for UNICAST responses. It immediately sends a multicast query and waits for 5s for responses. After 5s have elapsed it shuts down and repeats after 1 minute

This follows the implementation of go-libp2p-mdns except in go the interval is 0 seconds not 1 minute.

The JS implementation is untouched.

Can you please confirm that's cool and I'll get on with adding tests.

@jacobheun

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

@alanshaw that sounds reasonable to me.

I'll look at trying to get libp2p/specs#80 at least moved into Draft so those proper implementations can get unblocked. I believe rust-libp2p is already using that spec.

@alanshaw alanshaw marked this pull request as ready for review Apr 15, 2019

alanshaw and others added some commits Apr 8, 2019

feat: compatibility with go-libp2p-mdns
This PR adds a compatibility class that allows a js-libp2p node to find a go-libp2p node (and vice versa) over MDNS.

It's implemented as a separate class so the two differing implementations do not get confused.

I've verified this is working correctly by running a go-ipfs and js-ipfs node with no boostrap nodes (and no other discovery methods) and verifying they find each other.

TODO:

* [ ] Add tests!

Some tips if you want to try this out:

* After you've run `ipfs init`, remember to remove the bootstrap nodes from the config file (`~/.ipfs/config`) of each node before you start up
* Use `ipfs log level mdns debug` for some go-ipfs mdns logs
* You can use the following script (after `npm link`ing this branch) to start a js-ipfs node with no bootstrap nodes and no discovery modules other than MDNS:

```js
const IPFS = require('ipfs')
const MDNS = require('libp2p-mdns')
const TCP = require('libp2p-tcp')

const ipfs = new IPFS({
  repo: '/tmp/ipfs-mdns',
  config: {
    Bootstrap: []
  },
  libp2p: {
    modules: {
      peerDiscovery: [MDNS],
      transport: [TCP]
    }
  }
})

ipfs.on('ready', async () => {
  console.log('ipfs is ready')
  console.log('My Peer ID:', (await ipfs.id()).id)
  setInterval(async () => {
    const peers = await ipfs.swarm.peers()
    console.log(peers.length, 'peers:')
    peers.forEach(p => console.log(p.peer.toB58String()))
  }, 10000)
})
```

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
chore: appease linter
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
fix: move async to dependencies
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
fix: typo in comment
Co-Authored-By: alanshaw <alan.shaw@protocol.ai>
refactor: pr feedback
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
fix: respond directly to querier
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
fix: reemit the peer event from GoMulticastDNS
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
refactor: add interval between queries
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
chore: appease linter
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
fix: option name
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
fix: use async/nextTick
License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
fix: existing tests
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
test: add tests for querier and main compat class
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>

alanshaw added some commits Apr 15, 2019

chore: remove .only
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
test: add responder tests
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>

@alanshaw alanshaw force-pushed the feat/go-libp2p-mdns-compat branch from dc964c1 to 37fbfdd Apr 15, 2019

fix: increase timeout for query on interval test
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>

@alanshaw alanshaw requested a review from vasco-santos Apr 16, 2019

@lidel lidel referenced this pull request Apr 22, 2019

Open

Embedded JS-IPFS in Brave #716

17 of 38 tasks complete
@jacobheun
Copy link
Contributor

left a comment

Other than adding the compat option to the readme, this looks good.

@@ -18,10 +21,18 @@ class MulticastDNS extends EventEmitter {
this.port = options.port || 5353
this.peerInfo = options.peerInfo
this._queryInterval = null
this._onPeer = this._onPeer.bind(this)

if (options.compat !== false) {

This comment has been minimized.

Copy link
@jacobheun

jacobheun May 2, 2019

Contributor

Needs to be added to the readme.

docs: document new compat option
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>

@alanshaw alanshaw requested a review from jacobheun May 9, 2019

@alanshaw alanshaw referenced this pull request May 9, 2019

Closed

⚡️ v0.36.0 RELEASE 🚀 #2024

43 of 44 tasks complete
@vasco-santos
Copy link
Member

left a comment

LGTM!

@jacobheun
Copy link
Contributor

left a comment

Looks good. Just FYI, the mdns spec was merged https://github.com/libp2p/specs/blob/master/discovery/mdns.md. We can work with the go team to try and coordinate some future releases to the new version.

@jacobheun jacobheun merged commit c6d1d49 into master May 9, 2019

4 checks passed

Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
codecov/patch 98.22% of diff hit (target 94.62%)
Details
codecov/project 97.26% (+2.64%) compared to 92cfb26
Details

@jacobheun jacobheun deleted the feat/go-libp2p-mdns-compat branch May 9, 2019

@ghost ghost removed the in progress label May 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.