Fixing double callback firing. node 0.5 is much better about calling …

…errors on the client object which, when aborting on timeout, predictable emits an error which then triggers a double callback.
request · Oct 26, 2011 · a2e5d6e · a2e5d6e
1 parent 248e9d6
commit a2e5d6e
Show file tree

Hide file tree

Showing 2 changed files with 72 additions and 60 deletions.
diff --git a/main.js b/main.js
@@ -90,6 +90,17 @@ Request.prototype.getAgent = function (host, port) {
 }
 Request.prototype.request = function () {
   var self = this
+
+  // Protect against double callback
+  if (!self._callback && self.callback) {
+    self._callback = self.callback
+    self.callback = function () {
+      if (self._callbackCalled) return // Print a warning maybe?
+      self._callback.apply(self, arguments)
+      self._callbackCalled = true
+    }
+  }
+
   if (self.url) {
     // People use this property instead all the time so why not just support it.
     self.uri = self.url

diff --git a/tests/test-timeout.js b/tests/test-timeout.js
@@ -11,76 +11,77 @@ var remainingTests = 5;
 
 s.listen(s.port, function () {
   // Request that waits for 200ms
-s.on('/timeout', function (req, resp) {
-  setTimeout(function(){
-    resp.writeHead(200, {'content-type':'text/plain'})
-    resp.write(expectedBody)
-    resp.end()
-  }, 200);
-});
-
-// Scenario that should timeout
-var shouldTimeout = {
-  url: s.url + "/timeout",
-  timeout:100
-}
-
-request(shouldTimeout, function (err, resp, body) {
-  assert.equal(err.code, "ETIMEDOUT");
-  checkDone();
-})
+  s.on('/timeout', function (req, resp) {
+    setTimeout(function(){
+      resp.writeHead(200, {'content-type':'text/plain'})
+      resp.write(expectedBody)
+      resp.end()
+    }, 200);
+  });
 
+  // Scenario that should timeout
+  var shouldTimeout = {
+    url: s.url + "/timeout",
+    timeout:100
+  }
 
-// Scenario that shouldn't timeout
-var shouldntTimeout = {
-  url: s.url + "/timeout",
-  timeout:300
-}
 
-request(shouldntTimeout, function (err, resp, body) {
-  assert.equal(err, null);
-  assert.equal(expectedBody, body)
-  checkDone();
-})
+  request(shouldTimeout, function (err, resp, body) {
+    assert.equal(err.code, "ETIMEDOUT");
+    checkDone();
+  })
 
-// Scenario with no timeout set, so shouldn't timeout
-var noTimeout = {
-  url: s.url + "/timeout"
-}
 
-request(noTimeout, function (err, resp, body) {
-  assert.equal(err);
-  assert.equal(expectedBody, body)
-  checkDone();
-})
+  // Scenario that shouldn't timeout
+  var shouldntTimeout = {
+    url: s.url + "/timeout",
+    timeout:300
+  }
 
-// Scenario with a negative timeout value, should be treated a zero or the minimum delay
-var negativeTimeout = {
-  url: s.url + "/timeout",
-  timeout:-1000
-}
+  request(shouldntTimeout, function (err, resp, body) {
+    assert.equal(err, null);
+    assert.equal(expectedBody, body)
+    checkDone();
+  })
 
-request(negativeTimeout, function (err, resp, body) {
-  assert.equal(err.code, "ETIMEDOUT");
-  checkDone();
-})
+  // Scenario with no timeout set, so shouldn't timeout
+  var noTimeout = {
+    url: s.url + "/timeout"
+  }
 
-// Scenario with a float timeout value, should be rounded by setTimeout anyway
-var floatTimeout = {
-  url: s.url + "/timeout",
-  timeout: 100.76
-}
+  request(noTimeout, function (err, resp, body) {
+    assert.equal(err);
+    assert.equal(expectedBody, body)
+    checkDone();
+  })
 
-request(floatTimeout, function (err, resp, body) {
-  assert.equal(err.code, "ETIMEDOUT");
-  checkDone();
-})
+  // Scenario with a negative timeout value, should be treated a zero or the minimum delay
+  var negativeTimeout = {
+    url: s.url + "/timeout",
+    timeout:-1000
+  }
+
+  request(negativeTimeout, function (err, resp, body) {
+    assert.equal(err.code, "ETIMEDOUT");
+    checkDone();
+  })
+
+  // Scenario with a float timeout value, should be rounded by setTimeout anyway
+  var floatTimeout = {
+    url: s.url + "/timeout",
+    timeout: 100.76
+  }
+
+  request(floatTimeout, function (err, resp, body) {
+    assert.equal(err.code, "ETIMEDOUT");
+    checkDone();
+  })
 
-function checkDone() {
-  if(--remainingTests == 0) {
-    s.close();
-    console.log("All tests passed.");
+  function checkDone() {
+    if(--remainingTests == 0) {
+      s.close();
+      console.log("All tests passed.");
+    }
   }
-}
 })