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

seed / destroy race condition prevent destruction #254

Closed
mvayngrib opened this issue Jan 26, 2015 · 2 comments
Closed

seed / destroy race condition prevent destruction #254

mvayngrib opened this issue Jan 26, 2015 · 2 comments

Comments

@mvayngrib
Copy link
Member

@mvayngrib mvayngrib commented Jan 26, 2015

to repro:

client.seed()
client.destroy()

seed() calls self.add asynchronously and ends up adding to self.torrents after destroy empties it

I suggest something like bittorrent-dht has - self._destroyed with checks, and can implement it if you like. However, this will likely force changes to the ontorrent callback signature. Currently it's not a traditional node callback - ontorrent(torrent), but it will likely need to become one, a la:

if (self._destroyed) ontorrent && ontorrent(new Error('client is destroyed'))
@feross

This comment has been minimized.

Copy link
Member

@feross feross commented Jan 26, 2015

Good catch. I'll fix this up.

We don't actually need to make the ontorrent take an error callback. Calling destroy just means that the 'torrent' event will never get emitted. That's how it works with sockets and the 'listen' event in node.js. For example:

var net = require('net')
var socket = net.connect(4000, function () {
  console.log('listen')
})
socket.destroy()

will not log listen.

feross added a commit that referenced this issue Jan 26, 2015
@feross feross closed this in df77c94 Jan 26, 2015
@feross

This comment has been minimized.

Copy link
Member

@feross feross commented Jan 26, 2015

Fixed this issue, added tests, and released as 0.27.0.

@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked pull requests

Successfully merging a pull request may close this issue.

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