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

Option to disable BEP19 web seeds #911

Merged
merged 4 commits into from Sep 17, 2016
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -1,5 +1,9 @@
# WebTorrent Version History

## v0.97.0 - 2016-09-17

- Option to disable web seeds (BEP19)

## v0.95.2 - 2016-06-22

This comment has been minimized.

Copy link
@feross

feross Sep 17, 2016

Member

Looks like v0.96.0 is missing. I'll add it.

This comment has been minimized.

Copy link
@feross

feross Sep 17, 2016

Member

PR sent: #915

This comment has been minimized.

Copy link
@dcposch

dcposch Sep 18, 2016

Author Contributor

Left comments re some typos but otherwise looks good


- HEAD requests
 to HTTP server should not send entire body
@@ -51,11 +51,12 @@ If `opts` is specified, then the default options (shown below) will be overridde

```js
{
dht: Boolean|Object, // Enable DHT (default=true), or options object for DHT
maxConns: Number, // Max number of connections per torrent (default=55)
nodeId: String|Buffer, // DHT protocol node ID (default=randomly generated)
peerId: String|Buffer, // Wire protocol peer ID (default=randomly generated)
tracker: Boolean|Object // Enable trackers (default=true), or options object for Tracker
tracker: Boolean|Object, // Enable trackers (default=true), or options object for Tracker
dht: Boolean|Object, // Enable DHT (default=true), or options object for DHT
webSeeds: Boolean // Enable BEP19 web seeds (default=true)

This comment has been minimized.

Copy link
@feross

feross Sep 17, 2016

Member

This would have been better as singular, webSeed IMO since we didn't name the other option trackers. No worries, I'll change it along with the other breaking API changes in v1.0.

This comment has been minimized.

Copy link
@dcposch

dcposch Sep 18, 2016

Author Contributor

I wanted to match webConns. The other options, tracker and dht, both take an object, this one just takes true or false.

I think it's fine but have no strong opinion, if you want to change it in 1.0 that's cool too

}
```

@@ -139,6 +139,9 @@ function WebTorrent (opts) {
self.dht = false
}

// Enable or disable BEP19 (Web Seeds). Enabled by default:
self.enableWebSeeds = opts.webSeeds !== false

if (typeof loadIPSet === 'function' && opts.blocklist != null) {
loadIPSet(opts.blocklist, {
headers: {
@@ -471,9 +471,11 @@ Torrent.prototype._onMetadata = function (metadata) {
self.metadata = self.torrentFile

// add web seed urls (BEP19)
self.urlList.forEach(function (url) {
self.addWebSeed(url)
})
if (self.client.enableWebSeeds) {
self.urlList.forEach(function (url) {
self.addWebSeed(url)
})
}

// start off selecting the entire torrent with low priority
if (self.pieces.length !== 0) {
@@ -1,7 +1,7 @@
{
"name": "webtorrent",
"description": "Streaming torrent client",
"version": "0.96.5",
"version": "0.97.0",

This comment has been minimized.

Copy link
@feross

feross Sep 17, 2016

Member

In the future, please don't change the package.json version in a PR. That's best done at release time with the npm version command (i.e. npm version patch, npm version minor, npm version major).

This comment has been minimized.

Copy link
@feross

feross Sep 17, 2016

Member

If you're curious about the exact commands I use to do a release, they are here: https://github.com/feross/prezto/blob/master/runcoms/zshrc#L31-L33

This comment has been minimized.

Copy link
@feross

feross Sep 17, 2016

Member

Sorry -- I see now that you did make a separate commit for the version bump. Still, it's not good to encourage this in PRs. When random people send you a PR and increment the version, it makes it harder to merge that PR.

This comment has been minimized.

Copy link
@dcposch

dcposch Sep 18, 2016

Author Contributor

I did use npm version minor

I bumped the version because I need it for the desktop app: webtorrent/webtorrent-desktop#913

"author": {
"name": "WebTorrent, LLC",
"email": "feross@webtorrent.io",
@@ -9,8 +9,12 @@ var serveStatic = require('serve-static')
var test = require('tape')
var WebTorrent = require('../../')

// it should be fast to download a small torrent over local HTTP
var WEB_SEED_TIMEOUT_MS = 500

test('Download using webseed (via .torrent file)', function (t) {
t.plan(6)
t.timeoutAfter(WEB_SEED_TIMEOUT_MS)

This comment has been minimized.

Copy link
@feross

feross Sep 17, 2016

Member

Won't this just add flakiness to the test suite? We don't do this in any of the other tests. What if it takes 600ms to complete on Travis CI servers?

This comment has been minimized.

Copy link
@dcposch

dcposch Sep 18, 2016

Author Contributor

It completes very fast (~10ms) on my Macbook.

I added the timeout to match the timeout below. How do you verify that webSeeds: false is working? I thought: make sure that the webSeed download always completes in under X time, then set webSeeds: false and wait that same amount of time and verify there are no requests


var parsedTorrent = extend(fixtures.leaves.parsedTorrent)

@@ -72,3 +76,46 @@ test('Download using webseed (via .torrent file)', function (t) {
})
})
})

test('Disable webseeds', function (t) {
var parsedTorrent = extend(fixtures.leaves.parsedTorrent)

var httpServer = http.createServer(function (req, res) {
t.fail('webseed http server should not get any requests')
})
var client

httpServer.on('error', function (err) { t.fail(err) })

series([
function (cb) {
httpServer.listen(cb)
},

function (cb) {
parsedTorrent.urlList = [
'http://localhost:' + httpServer.address().port + '/' + fixtures.leaves.parsedTorrent.name
]

client = new WebTorrent({ dht: false, tracker: false, webSeeds: false })

client.on('error', function (err) { t.fail(err) })
client.on('warning', function (err) { t.fail(err) })

client.add(parsedTorrent, {store: MemoryChunkStore})

// The test above ensures that we can download the whole torrent over webseeds within a
// short time. Here, we wait the same amount of time and make sure no HTTP requests happen.

This comment has been minimized.

Copy link
@feross

feross Sep 17, 2016

Member

I'm not sure what this comment means. Since the webSeeds option was set to false, no download progress will be made, right? This will just timeout after 500ms and move on to the cleanup code below.

That's fine, I think waiting 500ms is a reasonable way to ensure that no http requests get sent to the server, but the comment should be changed to be clearer.

This comment has been minimized.

Copy link
@feross

feross Sep 17, 2016

Member

Also, another test we can add here is:

var torrent = client.add(parsedTorrent, {store: MemoryChunkStore})

torrent.on('ready', function () {
  t.equal(torrent.urlList.length, 0) // ensure no web seeds were added
})

This comment has been minimized.

Copy link
@dcposch

dcposch Sep 18, 2016

Author Contributor

"The test above" refers to the test that has webSeeds enabled. There are two tests in the file, the first ensures that web seeds work, the second ensures that webSeeds: false works

This comment has been minimized.

Copy link
@dcposch

dcposch Sep 18, 2016

Author Contributor

I don't think that extra test works. It doesn't prevent urlList from being populated (just as setting tracker: false won't make the announce list empty, right?). It just means that none of those URLs will be contacted.

setTimeout(cb, WEB_SEED_TIMEOUT_MS)
}
], function (err) {
t.error(err)
client.destroy(function (err) {
t.error(err, 'client destroyed')
})
httpServer.close(function () {
t.pass('http server closed')
t.end()
})
})
})
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.