Wait up to 1s for pending requests before destroy()

client.js

@@ -1,7 +1,7 @@
 module.exports = Client
 
 var Buffer = require('safe-buffer').Buffer
-var debug = require('debug')('bittorrent-tracker')
+var debug = require('debug')('bittorrent-tracker:client')
 var EventEmitter = require('events').EventEmitter
 var extend = require('xtend')
 var inherits = require('inherits')

lib/client/http-tracker.js

@@ -28,11 +28,16 @@ function HTTPTracker (client, announceUrl, opts) {
 
   // Determine scrape url (if http tracker supports it)
   self.scrapeUrl = null
-  var m
-  if ((m = self.announceUrl.match(HTTP_SCRAPE_SUPPORT))) {
-    self.scrapeUrl = self.announceUrl.slice(0, m.index) + '/scrape' +
-      self.announceUrl.slice(m.index + 9)
+
+  var match = self.announceUrl.match(HTTP_SCRAPE_SUPPORT)
+  if (match) {
+    var pre = self.announceUrl.slice(0, match.index)
+    var post = self.announceUrl.slice(match.index + 9)
+    self.scrapeUrl = pre + '/scrape' + post
   }
+
+  self.cleanupFns = []
+  self.maybeDestroyCleanup = null
 }
 
 HTTPTracker.prototype.DEFAULT_ANNOUNCE_INTERVAL = 30 * 60 * 1000 // 30 minutes
@@ -82,22 +87,61 @@ HTTPTracker.prototype.destroy = function (cb) {
   self.destroyed = true
   clearInterval(self.interval)
 
-  cb(null)
+  // If there are no pending requests, destroy immediately.
+  if (self.cleanupFns.length === 0) return destroyCleanup()
+
+  // Otherwise, wait a short time for pending requests to complete, then force
+  // destroy them.
+  var timeout = setTimeout(destroyCleanup, common.DESTROY_TIMEOUT)
+
+  // But, if all pending requests complete before the timeout fires, do cleanup
+  // right away.
+  self.maybeDestroyCleanup = function () {
+    if (self.cleanupFns.length === 0) destroyCleanup()
+  }
+
+  function destroyCleanup () {
+    if (timeout) {
+      clearTimeout(timeout)
+      timeout = null
+    }
+    self.maybeDestroyCleanup = null
+    self.cleanupFns.slice(0).forEach(function (cleanup) {
+      cleanup()
+    })
+    self.cleanupFns = []
+    cb(null)
+  }
 }
 
 HTTPTracker.prototype._request = function (requestUrl, params, cb) {
   var self = this
   var u = requestUrl + (requestUrl.indexOf('?') === -1 ? '?' : '&') +
     common.querystringStringify(params)
-  var opts = {
+
+  self.cleanupFns.push(cleanup)
+
+  var request = get.concat({
     url: u,
+    timeout: common.REQUEST_TIMEOUT,
     headers: {
       'user-agent': self.client._userAgent || ''
     }
+  }, onResponse)
+
+  function cleanup () {
+    if (request) {
+      self.cleanupFns.splice(self.cleanupFns.indexOf(cleanup), 1)
+      request.abort()
+      request = null
+    }
+    if (self.maybeDestroyCleanup) self.maybeDestroyCleanup()
   }
 
-  get.concat(opts, function (err, res, data) {
+  function onResponse (err, res, data) {
+    cleanup()
     if (self.destroyed) return
+
     if (err) return self.client.emit('warning', err)
     if (res.statusCode !== 200) {
       return self.client.emit('warning', new Error('Non-200 response code ' +
@@ -128,7 +172,7 @@ HTTPTracker.prototype._request = function (requestUrl, params, cb) {
     debug('response from ' + requestUrl)
 
     cb(data)
-  })
+  }
 }
 
 HTTPTracker.prototype._onAnnounceResponse = function (data) {

lib/client/udp-tracker.js

@@ -12,8 +12,6 @@ var url = require('url')
 var common = require('../common')
 var Tracker = require('./tracker')
 
-var TIMEOUT = 15000
-
 inherits(UDPTracker, Tracker)
 
 /**
@@ -29,6 +27,7 @@ function UDPTracker (client, announceUrl, opts) {
   debug('new udp tracker %s', announceUrl)
 
   self.cleanupFns = []
+  self.maybeDestroyCleanup = null
 }
 
 UDPTracker.prototype.DEFAULT_ANNOUNCE_INTERVAL = 30 * 60 * 1000 // 30 minutes
@@ -52,11 +51,31 @@ UDPTracker.prototype.destroy = function (cb) {
   self.destroyed = true
   clearInterval(self.interval)
 
-  self.cleanupFns.slice(0).forEach(function (cleanup) {
-    cleanup()
-  })
-  self.cleanupFns = []
-  cb(null)
+  // If there are no pending requests, destroy immediately.
+  if (self.cleanupFns.length === 0) return destroyCleanup()
+
+  // Otherwise, wait a short time for pending requests to complete, then force
+  // destroy them.
+  var timeout = setTimeout(destroyCleanup, common.DESTROY_TIMEOUT)
+
+  // But, if all pending requests complete before the timeout fires, do cleanup
+  // right away.
+  self.maybeDestroyCleanup = function () {
+    if (self.cleanupFns.length === 0) destroyCleanup()
+  }
+
+  function destroyCleanup () {
+    if (timeout) {
+      clearTimeout(timeout)
+      timeout = null
+    }
+    self.maybeDestroyCleanup = null
+    self.cleanupFns.slice(0).forEach(function (cleanup) {
+      cleanup()
+    })
+    self.cleanupFns = []
+    cb(null)
+  }
 }
 
 UDPTracker.prototype._request = function (opts) {
@@ -66,41 +85,51 @@ UDPTracker.prototype._request = function (opts) {
   var transactionId = genTransactionId()
   var socket = dgram.createSocket('udp4')
 
-  var cleanup = function () {
-    if (!socket) return
-    self.cleanupFns.splice(self.cleanupFns.indexOf(cleanup), 1)
-    if (timeout) {
-      clearTimeout(timeout)
-      timeout = null
-    }
-    socket.removeListener('error', onError)
-    socket.removeListener('message', onSocketMessage)
-    socket.on('error', noop) // ignore all future errors
-    try { socket.close() } catch (err) {}
-    socket = null
-  }
-  self.cleanupFns.push(cleanup)
-
-  // does not matter if `stopped` event arrives, so supress errors & cleanup after timeout
-  var ms = opts.event === 'stopped' ? TIMEOUT / 10 : TIMEOUT
   var timeout = setTimeout(function () {
-    timeout = null
+    // does not matter if `stopped` event arrives, so supress errors
     if (opts.event === 'stopped') cleanup()
     else onError(new Error('tracker request timed out (' + opts.event + ')'))
-  }, ms)
+    timeout = null
+  }, common.REQUEST_TIMEOUT)
   if (timeout.unref) timeout.unref()
 
+  self.cleanupFns.push(cleanup)
+
   send(Buffer.concat([
     common.CONNECTION_ID,
     common.toUInt32(common.ACTIONS.CONNECT),
     transactionId
   ]))
 
-  socket.on('error', onError)
+  socket.once('error', onError)
   socket.on('message', onSocketMessage)
 
-  function onSocketMessage (msg) {
+  function cleanup () {
+    if (timeout) {
+      clearTimeout(timeout)
+      timeout = null
+    }
+    if (socket) {
+      self.cleanupFns.splice(self.cleanupFns.indexOf(cleanup), 1)
+      socket.removeListener('error', onError)
+      socket.removeListener('message', onSocketMessage)
+      socket.on('error', noop) // ignore all future errors
+      try { socket.close() } catch (err) {}
+      socket = null
+    }
+    if (self.maybeDestroyCleanup) self.maybeDestroyCleanup()
+  }
+
+  function onError (err) {
+    cleanup()
     if (self.destroyed) return
+
+    if (err.message) err.message += ' (' + self.announceUrl + ')'
+    // errors will often happen if a tracker is offline, so don't treat it as fatal
+    self.client.emit('warning', err)
+  }
+
+  function onSocketMessage (msg) {
     if (msg.length < 8 || msg.readUInt32BE(4) !== transactionId.readUInt32BE(0)) {
       return onError(new Error('tracker sent invalid transaction id'))
     }
@@ -109,15 +138,20 @@ UDPTracker.prototype._request = function (opts) {
     debug('UDP response %s, action %s', self.announceUrl, action)
     switch (action) {
       case 0: // handshake
+        // Note: no check for `self.destroyed` so that pending messages to the
+        // tracker can still be sent/received even after destroy() is called
+
         if (msg.length < 16) return onError(new Error('invalid udp handshake'))
 
         if (opts._scrape) scrape(msg.slice(8, 16))
         else announce(msg.slice(8, 16), opts)
 
-        return
+        break
 
       case 1: // announce
         cleanup()
+        if (self.destroyed) return
+
         if (msg.length < 20) return onError(new Error('invalid announce message'))
 
         var interval = msg.readUInt32BE(8)
@@ -138,10 +172,13 @@ UDPTracker.prototype._request = function (opts) {
         addrs.forEach(function (addr) {
           self.client.emit('peer', addr)
         })
+
         break
 
       case 2: // scrape
         cleanup()
+        if (self.destroyed) return
+
         if (msg.length < 20 || (msg.length - 8) % 12 !== 0) {
           return onError(new Error('invalid scrape message'))
         }
@@ -158,12 +195,16 @@ UDPTracker.prototype._request = function (opts) {
             incomplete: msg.readUInt32BE(16 + (i * 12))
           })
         }
+
         break
 
       case 3: // error
         cleanup()
+        if (self.destroyed) return
+
         if (msg.length < 8) return onError(new Error('invalid error message'))
         self.client.emit('warning', new Error(msg.slice(8).toString()))
+
         break
 
       default:
@@ -172,14 +213,6 @@ UDPTracker.prototype._request = function (opts) {
     }
   }
 
-  function onError (err) {
-    if (self.destroyed) return
-    cleanup()
-    if (err.message) err.message += ' (' + self.announceUrl + ')'
-    // errors will often happen if a tracker is offline, so don't treat it as fatal
-    self.client.emit('warning', err)
-  }
-
   function send (message) {
     if (!parsedUrl.port) {
       parsedUrl.port = 80