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

Fix 'left' parameter on seed #1701

Merged
merged 4 commits into from Aug 18, 2019
Merged

Fix 'left' parameter on seed #1701

merged 4 commits into from Aug 18, 2019

Conversation

@alxhotel
Copy link
Member

alxhotel commented Aug 11, 2019

Fixes #1105

As mentioned by @feross, this PR just verifies the torrent before trying to discover peers.

I have tested it by seeding and downloading and it works fine.

If anyone has better solutions, let me know.

@feross

This comment has been minimized.

Copy link
Member

feross commented Aug 11, 2019

Thanks for taking a stab at solving this!

This doesn't seem to fix the issue for me. Piece verification is asynchronous so it is not guaranteed to complete before the Discovery object is initialized and sends messages to the tracker servers.

I used webtorrent-cli pointing to a version of webtorrent with this PR on it and got this:

DEBUG='bittorrent-tracker*' ./bin/cmd.js seed ~/Desktop/blah.png
  bittorrent-tracker:client new client c97eedde957e0689871a450f537c2cf19af36276 +0ms
  bittorrent-tracker:udp-tracker new udp tracker udp://explodie.org:6969 +0ms
  bittorrent-tracker:udp-tracker new udp tracker udp://tracker.coppersurfer.tk:6969 +0ms
  bittorrent-tracker:udp-tracker new udp tracker udp://tracker.empire-js.us:1337 +0ms
  bittorrent-tracker:udp-tracker new udp tracker udp://tracker.leechers-paradise.org:6969 +0ms
  bittorrent-tracker:udp-tracker new udp tracker udp://tracker.opentrackr.org:1337 +0ms
  bittorrent-tracker:client setInterval 900000 +2ms
  bittorrent-tracker:client send `start` { numwant: 50, uploaded: 0, downloaded: 0, left: 119515, event: 'started' } +1ms

Note that left is not 0. I think we would need to wait for the verification to actually complete.

@alxhotel

This comment has been minimized.

Copy link
Member Author

alxhotel commented Aug 12, 2019

Ah yeah, of course. Thanks for the feedback!

I've made some changes to take the asynchronous behaviour into account. (Still don't know if it's the best approach)

I also had to change the tests blocklist-dht and blocklist-tracker because the clients were sharing the download path. This meant that client2 always had the file downloaded, so it wasn't blocking any peers because it didn't need to. By separating the download paths, now the behaviour is as expected. (It was working previously because the discovery was always trigerring before verifying the pieces)

alxhotel added 2 commits Aug 12, 2019
@feross

This comment has been minimized.

Copy link
Member

feross commented Aug 18, 2019

the clients were sharing the download path

Thanks for catching this!

@feross
feross approved these changes Aug 18, 2019
Copy link
Member

feross left a comment

This is a perfect PR. Thank you @alxhotel ❤️

@feross feross merged commit 76c18cc into webtorrent:master Aug 18, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@feross

This comment has been minimized.

Copy link
Member

feross commented Aug 18, 2019

0.107.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.