Implement exact source (xs) for magnet URIs #799

Sebmaster · 2016-05-13T03:07:24Z

Since I saw #714 stalled a bit, I wanted to check in if no one's working on this and started off with a minimal working prototype.

I know I still need to move the fixture over (also don't rely on webtorrent.io, but create a local server instead) to the other repo, but does this look okay / is anyone else working on this?

feross · 2016-05-13T23:09:35Z

lib/torrent.js

+    try {
+      this._xsRequest = get.concat(opts, function (err, res, torrent) {
+        if (err || !torrent.length) return
+


Should check that res.statusCode === 200

feross · 2016-05-13T23:10:59Z

Just took a quick look. Looks pretty good to me. I don't think we to/can easily support ftp:// so I would remove that test case.

Sebmaster · 2016-05-13T23:13:28Z

Fixed.

I don't think we to/can easily support ftp:// so I would remove that test case.

I added that mainly to check if unsupported protocols would crash the process, since the spec says to silently ignore.

feross · 2016-05-13T23:15:01Z

I think we should check for http:// https:// explicitly before passing into simple-get. That would be more reliable and we could get rid of the try-catch around simple-get, right?

Sebmaster · 2016-05-13T23:23:41Z

Done.

That would be more reliable and we could get rid of the try-catch around simple-get, right?

For the protocol case yeah, just hope there's nothing else http will throw on...

Sebmaster · 2016-05-13T23:57:36Z

Okay, also migrated the tests over to a local server and reused the fixtures from webtorrent-fixtures. Should be ready (for xs, at least).


        Implement exact source (xs) for magnet URIs

feross · 2016-05-18T01:05:12Z

Thanks -- will take a look after getting WebTorrent Desktop v0.5 out.

Sebmaster · 2016-05-18T12:32:07Z

I just checked the malformed URL case some more - a URL like http://example.org:2455665555/ would now crash, which isn't great, so I'm thinking leaving the try / catch there would be better (either instead, or in addition to the protocol check).

feross · 2016-05-19T00:18:19Z

lib/torrent.js

+
+  if (parsedTorrent.xs && !self.info) {
+    // TODO: Could try multiple in parallel here
+    var src = parsedTorrent.xs instanceof Array ? parsedTorrent.xs[0] : parsedTorrent.xs


Array.isArray() is the right way to check if something is an Array.

feross · 2016-05-19T00:20:01Z

lib/torrent.js

+        }
+      }
+      this._xsRequest = get.concat(opts, function (err, res, torrent) {
+        if (err || res.statusCode !== 200 || !torrent.length) return


No need to check for !torrent.length since that will make parseTorrent throw below anyway.

Would be nice to add a log here with debug for debugging purposes.

feross · 2016-05-19T00:20:43Z

lib/torrent.js

+
+        if (parsedTorrent) {
+          if (parsedTorrent.infoHash !== self.infoHash) {
+            return


Debug log here too

feross · 2016-05-19T00:27:38Z

lib/torrent.js

+  if (self._xsRequest) {
+    self._xsRequest.abort()
+    self._xsRequest = null
+  }


This should go after the next line.

feross · 2016-05-19T00:30:32Z

test/node/download-metadata.js

+
+  createServer(fixtures.leaves.torrent, function (url, next) {
+    client.add(fixtures.leaves.magnetURI + '&xs=' + encodeURIComponent(url), function (torrent) {
+      t.equal(torrent.name, 'Leaves of Grass by Walt Whitman.epub')


The name is also in the magnet URI as the &dn parameter. Might be better to check for another parameter.

feross · 2016-05-19T00:31:31Z

test/node/download-metadata.js

+  var client = new WebTorrent({ dht: false, tracker: false })
+  client.add(fixtures.leaves.magnetURI + '&xs=' + encodeURIComponent('invalidurl:example'))
+  setTimeout(function () {
+    t.ok(true, 'no crash')


No need for this. The tests will fail if there's a thrown exception. We should add listeners for 'error' and 'warning' events to all these tests though.

Nevermind, I see what you were doing.

Sebmaster · 2016-05-19T00:42:53Z

I totally fixed all of those just now 😄

Also what about the try-catch situation with malformed urls?

feross · 2016-05-19T00:46:53Z

Ah, thanks. I'm fixing all that up too. Sorry, should have communicated that better.

feross · 2016-05-19T00:47:09Z

I'm adding support for multiple &xs= params too.


        Fixes for PR #799

- Support multiple &xs= params in parallel - Fail on 'error' 'warning' events

feross · 2016-05-19T06:02:32Z

@Sebmaster See #809


        Merge pull request #809 from feross/fixes-799

Fixes for PR #799

lock · 2018-05-04T08:49:54Z

This thread has been automatically locked because it has not had recent activity. To discuss futher, please open a new issue.

Sebmaster force-pushed the Sebmaster:feature/magnet-xs branch 3 times, most recently from 1bfa128 to 0b814ae May 13, 2016

feross reviewed May 13, 2016
View changes

lib/torrent.js Outdated

try {

this._xsRequest = get.concat(opts, function (err, res, torrent) {

if (err || !torrent.length) return

This comment has been minimized.

Sign in to view

Copy link

feross May 13, 2016
Member

Should check that res.statusCode === 200

Sebmaster force-pushed the Sebmaster:feature/magnet-xs branch from 0b814ae to fc3c472 May 13, 2016

Sebmaster force-pushed the Sebmaster:feature/magnet-xs branch from fc3c472 to 7d5383c May 13, 2016

Sebmaster force-pushed the Sebmaster:feature/magnet-xs branch 4 times, most recently from ef86e35 to 8288c3b May 13, 2016

Sebmaster force-pushed the Sebmaster:feature/magnet-xs branch 2 times, most recently from 2162641 to 9b2335a May 14, 2016

Implement exact source (xs) for magnet URIs

Loading status checks…

d498f11

Sebmaster force-pushed the Sebmaster:feature/magnet-xs branch from 9b2335a to d498f11 May 17, 2016

feross reviewed May 19, 2016
View changes

lib/torrent.js

if (parsedTorrent) {

if (parsedTorrent.infoHash !== self.infoHash) {

return

This comment has been minimized.

Sign in to view

Copy link

feross May 19, 2016
Member

Debug log here too

feross reviewed May 19, 2016
View changes

lib/torrent.js

if (self._xsRequest) {

self._xsRequest.abort()

self._xsRequest = null

}

This comment has been minimized.

Sign in to view

Copy link

feross May 19, 2016
Member

This should go after the next line.

feross reviewed May 19, 2016
View changes

feross merged commit 3894024 into webtorrent:master May 19, 2016
1 of 2 checks passed

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

feross added a commit that referenced this pull request May 19, 2016

Fixes for PR #799

Loading status checks…

e8dea8e

- Support multiple &xs= params in parallel - Fail on 'error' 'warning' events

feross added a commit that referenced this pull request May 19, 2016

Merge pull request #809 from feross/fixes-799

Loading status checks…

4f30a99

Fixes for PR #799

lock bot locked as resolved and limited conversation to collaborators May 4, 2018

webtorrent / webtorrent

Implement exact source (xs) for magnet URIs #799

Implement exact source (xs) for magnet URIs #799

Sebmaster commented May 13, 2016

This comment has been minimized.

This comment has been minimized.

feross commented May 13, 2016

This comment has been minimized.

Sebmaster commented May 13, 2016

This comment has been minimized.

feross commented May 13, 2016

This comment has been minimized.

Sebmaster commented May 13, 2016

This comment has been minimized.

Sebmaster commented May 13, 2016

This comment has been minimized.

feross commented May 18, 2016

This comment has been minimized.

Sebmaster commented May 18, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Sebmaster commented May 19, 2016

This comment has been minimized.

feross commented May 19, 2016

This comment has been minimized.

feross commented May 19, 2016

This comment has been minimized.

feross commented May 19, 2016

This comment has been minimized.

lock bot commented May 4, 2018

webtorrent / webtorrent

Sponsor webtorrent/webtorrent

Join GitHub today

Implement exact source (xs) for magnet URIs #799

Implement exact source (xs) for magnet URIs #799

Conversation

Sebmaster commented May 13, 2016

This comment has been minimized.

This comment has been minimized.

feross commented May 13, 2016

This comment has been minimized.

Sebmaster commented May 13, 2016

This comment has been minimized.

feross commented May 13, 2016

This comment has been minimized.

Sebmaster commented May 13, 2016

This comment has been minimized.

Sebmaster commented May 13, 2016

This comment has been minimized.

feross commented May 18, 2016

This comment has been minimized.

Sebmaster commented May 18, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Sebmaster commented May 19, 2016

This comment has been minimized.

feross commented May 19, 2016

This comment has been minimized.

feross commented May 19, 2016

This comment has been minimized.

feross commented May 19, 2016

This comment has been minimized.

lock bot commented May 4, 2018