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

Don't send 'completed' event to tracker on client.seed #991

Merged
merged 2 commits into from Jan 18, 2017

Conversation

@sidd
Copy link
Contributor

sidd commented Dec 6, 2016

Follow-on to bittorrent-tracker/#185

Completed event now only emits when a previously started torrent download completes

Copy link
Member

DiegoRBaquero left a comment

Shouldn't we skip calling _checkDone in the first place?

@sidd

This comment has been minimized.

Copy link
Contributor Author

sidd commented Dec 7, 2016

@DiegoRBaquero From a glance it looks like _checkDone is eventually called via _onTorrentId, but this is from client.seed. So we could remove _checkDone from torrent.load, but now torrent.js's proper operation is dependent on properly using it through client.seed

I'm not very close to this code, so let me know if I'm missing something obvious!

@feross

This comment has been minimized.

Copy link
Member

feross commented Dec 8, 2016

@DiegoRBaquero _checkDone needs to get called or else file.on('done') and torrent.on('done') won't fire on torrents that are added when they are already complete on disk.

I think this PR is correct to just change the message that is sent to the tracker server.

However, I have style comments. See inline.

function onUpdateTick () {
process.nextTick(function () { self._update() })
}

return true
}

Torrent.prototype._checkDone = function () {
Torrent.prototype._checkDone = function (cb) {

This comment has been minimized.

Copy link
@feross

feross Dec 8, 2016

Member

No need for a callback. This method is entirely sync. Let's just return true if the entire torrent is done, false otherwise.

@@ -1579,7 +1584,7 @@ Torrent.prototype._checkDone = function () {
if (!self.done && done) {
self.done = true
self._debug('torrent done: ' + self.infoHash)
self.discovery.complete()
cb(null)
self.emit('done')

This comment has been minimized.

Copy link
@feross

feross Dec 8, 2016

Member

Let's return true at the very end of this if statement.

This comment has been minimized.

Copy link
@sidd

sidd Dec 8, 2016

Author Contributor

Returning true before _gcSelections seems to be causing tests to hang. getBuffer never calls its callback: https://github.com/feross/webtorrent/blob/master/test/node/download-dht-magnet.js#L77. return done after _gcSelections the function resolves this. Thoughts?

This comment has been minimized.

Copy link
@feross

feross Jan 18, 2017

Member

Yeah, good point

@@ -1579,7 +1584,7 @@ Torrent.prototype._checkDone = function () {
if (!self.done && done) {
self.done = true
self._debug('torrent done: ' + self.infoHash)
self.discovery.complete()
cb(null)
self.emit('done')
}

This comment has been minimized.

Copy link
@feross

feross Dec 8, 2016

Member

Otherwise, return false.

@@ -1532,7 +1532,7 @@ Torrent.prototype._request = function (wire, index, hotswap) {
wire.have(index)
})

self._checkDone()
self._checkDone(onDone)

This comment has been minimized.

Copy link
@feross

feross Dec 8, 2016

Member

If this returns true, then we can call self.discovery.complete().

@feross
feross approved these changes Jan 18, 2017
@feross feross merged commit 3ef2289 into webtorrent:master Jan 18, 2017
2 of 3 checks passed
2 of 3 checks passed
Node Security Something unexpected happened
Details
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 Jan 18, 2017

0.98.2

@lock

This comment has been minimized.

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.
Linked issues

Successfully merging this pull request may close these issues.

None yet

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