Skip to content

/ webtorrent

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

Do not choke on web seeds #972

Merged
merged 3 commits into from Nov 27, 2016
Merged

Do not choke on web seeds #972

merged 3 commits into from Nov 27, 2016

Conversation

@gpetrov
Copy link
Contributor

gpetrov commented Nov 6, 2016

Web seeds should be considered as pure uncheked seeds according to BEP19

So we should never choke on them. Otherwise when there are no other
seeds, the downloads will hang
@gpetrov
Do not choke on web seeds
Loading status checks…
a6a52cf 
Web seeds should be considered as pure uncheked seeds according to BEP19

So we should never choke on them. Otherwise when there are no other
seeds, the downloads will hang
This was referenced Nov 6, 2016
Closed
Closed
@josephfrazier
josephfrazier approved these changes Nov 11, 2016
View changes
Copy link
Member

josephfrazier left a comment

I have one inline nitpick, but this LGTM regardless :)
lib/torrent.js
@@ -1093,8 +1093,10 @@ Torrent.prototype._onWireWithMetadata = function (wire) {
wire.port(self.client.dht.address().port)
}

timeoutId = setTimeout(onChokeTimeout, CHOKE_TIMEOUT)
if (timeoutId.unref) timeoutId.unref()
if (wire.type !== 'webSeed') { // do not choke on webseeds

This comment has been minimized.

Sign in to view
Copy link
@josephfrazier

josephfrazier Nov 11, 2016

Member

Given that onChokeTimeout will do nothing if wire.type === 'webSeed', is this guard necessary?

This comment has been minimized.

Sign in to view
Copy link
@DiegoRBaquero

DiegoRBaquero Nov 11, 2016

Member

I'd rather have this line and remove the change in onChokeTimeout. Never setting the timeout is better.

This comment has been minimized.

Sign in to view
Copy link
@josephfrazier

josephfrazier Nov 11, 2016

Member

Good point. I noticed there's one other place where the timeout could be set, but I'm assuming it won't actually happen for web seeds? https://github.com/gpetrov/webtorrent/blob/a6a52cf436dfaa078dfa58dabcc2d4ac01bbbf2a/lib/torrent.js#L1070

This comment has been minimized.

Sign in to view
Copy link
@DiegoRBaquero

DiegoRBaquero Nov 11, 2016

Member

Nope, a webseed can't send a "choke" signal.

This comment has been minimized.

Sign in to view
Copy link
@feross

feross Nov 12, 2016

Member

I agree with this. Let's not set the timeout

This comment has been minimized.

Sign in to view
Copy link
@gpetrov

gpetrov Nov 12, 2016

Author Contributor

After further diving into the webseed logic, I think it should be even better if the webseed kicks in as last, when for example there are no other peers. Currently if webseed is given it is directly used as first peer and when this is a fast website connection, most downloads will go through the webseed and other peers will be used very little.

So I would suggest that webseeds also get the lowest priority (maybe as option).

I solved this currently in my case by adding the webseed dynamically 3 seconds after the torrent has being activated, so all client peers are exhausted first, but it is not an optimal solution.

What do you think?

This comment has been minimized.

Sign in to view
Copy link
@josephfrazier

josephfrazier Nov 17, 2016

Member

I'm not too familiar with the logic/code that handles webseed priority, but it sounds like a technically separate issue. I think we should go ahead and land this PR (once the required change has been made), then discuss webseed priority in a new issue/PR :-)
@feross
feross approved these changes Nov 12, 2016
View changes
lib/torrent.js
@@ -1093,8 +1093,10 @@ Torrent.prototype._onWireWithMetadata = function (wire) {
wire.port(self.client.dht.address().port)
}

timeoutId = setTimeout(onChokeTimeout, CHOKE_TIMEOUT)
if (timeoutId.unref) timeoutId.unref()
if (wire.type !== 'webSeed') { // do not choke on webseeds

This comment has been minimized.

Sign in to view
Copy link
@feross

feross Nov 12, 2016

Member

I agree with this. Let's not set the timeout
@feross

This comment has been minimized.

Sign in to view
Copy link
Member

feross commented Nov 12, 2016 •
edited

After the PR is updated to address the feedback, feel free to land this PR
gpetrov added 2 commits Nov 27, 2016
@gpetrov
Merge remote-tracking branch 'feross/master'
Loading status checks…
2db5ec7
@gpetrov
No need to check for webSeed in onChokeTimeout
Loading status checks…
7ad2f4c 
as the timeout is never set for webSeeds now
@gpetrov

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

gpetrov commented Nov 27, 2016

are we good now? :)
@josephfrazier josephfrazier merged commit 4979721 into webtorrent:master Nov 27, 2016
3 checks passed
3 checks passed
Node Security No known vulnerabilities found
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@josephfrazier

This comment has been minimized.

Sign in to view
Copy link
Member

josephfrazier commented Nov 27, 2016

Looks great, thanks!
@DiegoRBaquero

This comment has been minimized.

Sign in to view
Copy link
Member

DiegoRBaquero commented Nov 27, 2016

Great! Something we needed. @feross Can we get this published?
@feross

This comment has been minimized.

Sign in to view
Copy link
Member

feross commented Jan 13, 2017

@DiegoRBaquero Sorry for the delay. Just published this as 0.98.1.
@feross feross mentioned this pull request Jan 19, 2017
Closed
@lock

This comment has been minimized.

Sign in to view
Copy link

lock bot commented May 4, 2018

This thread has been automatically locked because it has not had recent activity. To discuss futher, please open a new issue.
@lock lock bot locked as resolved and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@DiegoRBaquero DiegoRBaquero

@josephfrazier josephfrazier

@feross feross

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@gpetrov @feross @josephfrazier @DiegoRBaquero
You can’t perform that action at this time.