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

Do not choke on web seeds #972

Merged
merged 3 commits into from Nov 27, 2016
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Next

Do not choke on web seeds

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
  • Loading branch information
gpetrov committed Nov 6, 2016
commit a6a52cf436dfaa078dfa58dabcc2d4ac01bbbf2a
@@ -1026,7 +1026,7 @@ Torrent.prototype._onWireWithMetadata = function (wire) {
var timeoutId = null

function onChokeTimeout () {
if (self.destroyed || wire.destroyed) return
if (self.destroyed || wire.destroyed || wire.type === 'webSeed') return

if (self._numQueued > 2 * (self._numConns - self.numPeers) &&
wire.amInterested) {
@@ -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.

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.

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.

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.

Copy link
@DiegoRBaquero

DiegoRBaquero Nov 11, 2016

Member

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

This comment has been minimized.

Copy link
@feross

feross Nov 12, 2016

Member

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

This comment has been minimized.

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.

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 :-)

timeoutId = setTimeout(onChokeTimeout, CHOKE_TIMEOUT)
if (timeoutId.unref) timeoutId.unref()
}

wire.isSeeder = false
updateSeedStatus()
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.