Fix a bug in http.Client where parsers may be prematurely released back to the free...

diff --git a/lib/http.js b/lib/http.js
index a02a75f..3f348ad 100644 (file)
--- a/lib/http.js
+++ b/lib/http.js
@@ -623,10 +623,23 @@ function Client ( ) {
 
   var requests = [];
   var currentRequest;
+  var parser;
 
-  var parser = parsers.alloc();
-  parser.reinitialize('response');
-  parser.socket = this;
+  self._initParser = function () {
+    if (!parser) parser = parsers.alloc();
+    parser.reinitialize('response');
+    parser.socket = self;
+    parser.onIncoming = function (res) {
+      debug("incoming response!");
+
+      res.addListener('end', function ( ) {
+        debug("request complete disconnecting. readyState = " + self.readyState);
+        self.end();
+      });
+
+      currentRequest.emit("response", res);
+    };
+  };
 
   self._reconnect = function () {
     if (self.readyState != "opening") {
@@ -650,6 +663,9 @@ function Client ( ) {
   };
 
   self.ondata = function (d, start, end) {
+    if (!parser) {
+      throw new Error("parser not initialized prior to Client.ondata call");
+    }
     var bytesParsed = parser.execute(d, start, end - start);
     if (parser.incoming && parser.incoming.upgrade) {
       var upgradeHead = d.slice(start + bytesParsed, end - start);
@@ -665,7 +681,7 @@ function Client ( ) {
     if (this.https) {
       this.setSecure(this.credentials);
     } else {
-      parser.reinitialize('response');
+      self._initParser();
       debug('requests: ' + sys.inspect(requests));
       currentRequest = requests.shift()
       currentRequest.flush();
@@ -673,7 +689,7 @@ function Client ( ) {
   });
 
   self.addListener("secure", function () {
-    parser.reinitialize('response');
+    self._initParser();
     debug('requests: ' + sys.inspect(requests));
     currentRequest = requests.shift()
     currentRequest.flush();
@@ -694,21 +710,12 @@ function Client ( ) {
     // If there are more requests to handle, reconnect.
     if (requests.length > 0) {
       self._reconnect();
-    } else {
+    } else if (parser) {
       parsers.free(parser);
+      parser = null;
     }
   });
 
-  parser.onIncoming = function (res) {
-    debug("incoming response!");
-
-    res.addListener('end', function ( ) {
-      debug("request complete disconnecting. readyState = " + self.readyState);
-      self.end();
-    });
-
-    currentRequest.emit("response", res);
-  };
 };
 sys.inherits(Client, net.Stream);
 

diff --git a/test/simple/test-http-client-race-2.js b/test/simple/test-http-client-race-2.js
new file mode 100644 (file)
index 0000000..ba7a28b
--- /dev/null
+++ b/test/simple/test-http-client-race-2.js
@@ -0,0 +1,95 @@
+require("../common");
+http = require("http");
+url = require("url");
+
+//
+// Slight variation on test-http-client-race to test for another race
+// condition involving the parsers FreeList used internally by http.Client.
+//
+
+var body1_s = "1111111111111111";
+var body2_s = "22222";
+var body3_s = "3333333333333333333";
+
+var server = http.createServer(function (req, res) {
+  var pathname = url.parse(req.url).pathname;
+
+  var body;
+  switch (pathname) {
+    case "/1": body = body1_s; break;
+    case "/2": body = body2_s; break;
+    default: body = body3_s;
+  };
+
+  res.writeHead(200, { "Content-Type": "text/plain"
+                      , "Content-Length": body.length
+                      });
+  res.end(body);
+});
+server.listen(PORT);
+
+var client = http.createClient(PORT);
+
+var body1 = "";
+var body2 = "";
+var body3 = "";
+
+//
+// Client #1 is assigned Parser #1
+//
+var req1 = client.request("/1")
+req1.addListener('response', function (res1) {
+  res1.setBodyEncoding("utf8");
+
+  res1.addListener('data', function (chunk) {
+    body1 += chunk;
+  });
+
+  res1.addListener('end', function () {
+    //
+    // Delay execution a little to allow the "close" event to be processed
+    // (required to trigger this bug!)
+    //
+    setTimeout(function () {
+      //
+      // The bug would introduce itself here: Client #2 would be allocated the
+      // parser that previously belonged to Client #1. But we're not finished
+      // with Client #1 yet!
+      //
+      var client2 = http.createClient(PORT);
+
+      //
+      // At this point, the bug would manifest itself and crash because the
+      // internal state of the parser was no longer valid for use by Client #1.
+      //
+      var req2 = client.request("/2");
+      req2.addListener('response', function (res2) {
+        res2.setBodyEncoding("utf8");
+        res2.addListener('data', function (chunk) { body2 += chunk; });
+        res2.addListener('end', function () {
+
+          //
+          // Just to be really sure we've covered all our bases, execute a
+          // request using client2.
+          //
+          var req3 = client2.request("/3");
+          req3.addListener('response', function (res3) {
+            res3.setBodyEncoding("utf8");
+            res3.addListener('data', function (chunk) { body3 += chunk });
+            res3.addListener('end', function() { server.close(); });
+          });
+          req3.end();
+        });
+      });
+      req2.end();
+    }, 500);
+  });
+});
+req1.end();
+
+process.addListener("exit", function () {
+  assert.equal(body1_s, body1);
+  assert.equal(body2_s, body2);
+  assert.equal(body3_s, body3);
+});
+