mafintosh / torrent-stream

The low level streaming torrent engine that peerflix uses
MIT License
1.94k stars 228 forks source link

Update to bittorrent-dht 1.0.0 #68

Closed feross closed 9 years ago

feross commented 10 years ago

I just updated bittorrent-dht to 1.0.0. It now follows the spec, contains a proper routing table, answers find_node requests from other nodes, and more efficiently crawls the DHT.

Previously, to find peers we would do a very aggressive crawl where we stuck every new node into a big queue and queried them all, not paying any attention to how close they were to the target info hash. So we had lots of pointless traffic to far away nodes that weren't helping us at all. Now, we only recurse on the K closest nodes, so we find the correct nodes with a lot less UDP traffic.

There may be some perf regressions or problems, so it would be great if you could update peerflix and let me know if you find any bugs.

feross commented 10 years ago

I should mention that the API has changed slightly. Take a look at the docs.

In short:

mafintosh commented 10 years ago

Wow great job! If only npm wasn't throwing 503s I would fix this and publish right away

mafintosh commented 10 years ago

Updated torrent-stream to use the new interface and released this in 0.13.1 (with a bump to bittorrent-tracker 1.5.0 as well). Initial testing seems to indicate great performance!

mafintosh commented 10 years ago

I'm gonna leave this open for a while is case we run into problems/regressions etc. /cc @asapach @galedric

asapach commented 10 years ago

Seems to work fine. Haven't noticed any problems so far.

BastienClement commented 10 years ago

Haven't noticed any problems with this either. Didn't tested latest patches however.

feross commented 10 years ago

Wow, that's great! I rewrote it from scratch so I was expecting at least a couple issues.

On Saturday, June 14, 2014, Bastien Clément notifications@github.com wrote:

Haven't noticed any problems with this either. Didn't tested latest patches however.

— Reply to this email directly or view it on GitHub https://github.com/mafintosh/torrent-stream/issues/68#issuecomment-46101338 .

Feross | blog http://feross.org/ | webtorrent http://webtorrent.io/ | studynotes http://www.apstudynotes.org/

Ayms commented 10 years ago

This seems to be working well but in addition to remark #74 I noticed that the same peer can be attempted to be added several time, if think something is wrong with the exist check in _addPeer, and as @feross commented in the code I don't think this check is optimal, I would suggest something like:

DHT.prototype._addPeer = function (addr, infoHash) {
  var self = this
  if (self._destroyed) return

  infoHash = idToHexString(infoHash)

  if (!self.peers[addr]) {
    self.peers[addr]=infoHash; //or whatever
    self.emit('peer', addr, infoHash)
    debug('adding peer ' + addr + ' ' + infoHash)
 }
}

and then corresponding changes in other functions

Since it creates a flow of useless messages/connections I think you should implement a fix asap.

feross commented 10 years ago

@Ayms Just confim, are you referring the behavior where the 'peer' event gets fired multiple times for the same peer? I just pushed a fix to that.

Also, in the future, please open an issue directly on bittorrent-dht so I won't miss it.

Ayms commented 10 years ago

@feross, yes, I saw your fix on bittorrent-dht, I will open new issues if any on bittorrent-dht.

Maybe there are good reasons for the code to be as it is and to manipulate arrays, which looks expensive, but let's correct anyway my above suggestion:

DHT.prototype._addPeer = function (addr, infoHash, from) {
  var self = this
  if (self._destroyed) return

  infoHash = idToHexString(infoHash)

  var peers = self.peers[infoHash]
  if (!peers) {
    peers = self.peers[infoHash] = {}
  }

  if (!peers[addr]) {
    peers[addr]=/true or something you might need/;
    self.emit('peer', addr, infoHash, from)
    debug('adding peer ' + addr + ' ' + infoHash + ' discovered from ' + from)
  }
}
mafintosh commented 9 years ago

closing this as the tracker works great!