http: Raise hangup error on destroyed socket write

Prior to v0.10, Node ignored ECONNRESET errors in many situations.
There *are* valid cases in which ECONNRESET should be ignored as a
normal part of the TCP dance, but in many others, it's a very relevant
signal that must be heeded with care.

Exacerbating this problem, if the OutgoingMessage does not have a
req.connection._handle, it assumes that it is in the process of
connecting, and thus buffers writes up in an array.

The problem happens when you reuse a socket between two requests, and it
is destroyed abruptly in between them.  The writes will be buffered,
because the socket has no handle, but it's not ever going to GET a
handle, because it's not connecting, it's destroyed.

The proper fix is to treat ECONNRESET correctly.  However, this is a
behavior/semantics change, and cannot land in a stable branch.

Fix #4775

diff --git a/lib/http.js b/lib/http.js
index 315a9c6a249c5335a45fa7cb566b594493bf54ce..6f3eff1ba8c699d537849647c2f9593a076e8baf 100644 (file)
--- a/lib/http.js
+++ b/lib/http.js
@@ -484,7 +484,8 @@ OutgoingMessage.prototype._writeRaw = function(data, encoding) {
 
   if (this.connection &&
       this.connection._httpMessage === this &&
-      this.connection.writable) {
+      this.connection.writable &&
+      !this.connection.destroyed) {
     // There might be pending data in the this.output buffer.
     while (this.output.length) {
       if (!this.connection.writable) {
@@ -498,7 +499,16 @@ OutgoingMessage.prototype._writeRaw = function(data, encoding) {
 
     // Directly write to socket.
     return this.connection.write(data, encoding);
+  } else if (this.connection && this.connection.destroyed) {
+    // The socket was destroyed.  If we're still trying to write to it,
+    // then something bad happened.
+    // If we've already raised an error on this message, then just ignore.
+    if (!this._hadError) {
+      this.emit('error', createHangUpError());
+      this._hadError = true;
+    }
   } else {
+    // buffer, as long as we're not destroyed.
     this._buffer(data, encoding);
     return false;
   }
@@ -1398,8 +1408,17 @@ function socketCloseListener() {
     // receive a response. The error needs to
     // fire on the request.
     req.emit('error', createHangUpError());
+    req._hadError = true;
   }
 
+  // Too bad.  That output wasn't getting written.
+  // This is pretty terrible that it doesn't raise an error.
+  // Fixed better in v0.10
+  if (req.output)
+    req.output.length = 0;
+  if (req.outputEncodings)
+    req.outputEncodings.length = 0;
+
   if (parser) {
     parser.finish();
     freeParser(parser, req);

diff --git a/test/simple/test-http-destroyed-socket-write.js b/test/simple/test-http-destroyed-socket-write.js
new file mode 100644 (file)
index 0000000..c562987
--- /dev/null
+++ b/test/simple/test-http-destroyed-socket-write.js
@@ -0,0 +1,131 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var common = require('../common');
+var assert = require('assert');
+
+// Fix the memory explosion that happens when writing to a http request
+// where the server has destroyed the socket on us between a successful
+// first request, and a subsequent request that reuses the socket.
+//
+// This test should not be ported to v0.10 and higher, because the
+// problem is fixed by not ignoring ECONNRESET in the first place.
+
+var http = require('http');
+var net = require('net');
+var server = http.createServer(function(req, res) {
+  // simulate a server that is in the process of crashing or something
+  // it only crashes after the first request, but before the second,
+  // which reuses the connection.
+  res.end('hallo wereld\n', function() {
+    setTimeout(function() {
+      req.connection.destroy();
+    }, 100);
+  });
+});
+
+var gotFirstResponse = false;
+var gotFirstData = false;
+var gotFirstEnd = false;
+server.listen(common.PORT, function() {
+
+  var gotFirstResponse = false;
+  var first = http.request({
+    port: common.PORT,
+    path: '/'
+  });
+  first.on('response', function(res) {
+    gotFirstResponse = true;
+    res.on('data', function(chunk) {
+      gotFirstData = true;
+    });
+    res.on('end', function() {
+      gotFirstEnd = true;
+    })
+  });
+  first.end();
+  second();
+
+  function second() {
+    var sec = http.request({
+      port: common.PORT,
+      path: '/',
+      method: 'POST'
+    });
+
+    var timer = setTimeout(write, 200);
+    var writes = 0;
+    var sawFalseWrite;
+
+    var gotError = false;
+    sec.on('error', function(er) {
+      assert.equal(gotError, false);
+      gotError = true;
+      assert(er.code === 'ECONNRESET');
+      clearTimeout(timer);
+      test();
+    });
+
+    function write() {
+      if (++writes === 64) {
+        clearTimeout(timer);
+        sec.end();
+        test();
+      } else {
+        timer = setTimeout(write);
+        var writeRet = sec.write(new Buffer('hello'));
+
+        // Once we find out that the connection is destroyed, every
+        // write() returns false
+        if (sawFalseWrite)
+          assert.equal(writeRet, false);
+        else
+          sawFalseWrite = writeRet === false;
+      }
+    }
+
+    assert.equal(first.connection, sec.connection,
+                 'should reuse connection');
+
+    sec.on('response', function(res) {
+      res.on('data', function(chunk) {
+        console.error('second saw data: ' + chunk);
+      });
+      res.on('end', function() {
+        console.error('second saw end');
+      });
+    });
+
+    function test() {
+      server.close();
+      assert(sec.connection.destroyed);
+      if (sec.output.length || sec.outputEncodings.length)
+        console.error('bad happened', sec.output, sec.outputEncodings);
+      assert.equal(sec.output.length, 0);
+      assert.equal(sec.outputEncodings, 0);
+      assert(gotError);
+      assert(gotFirstResponse);
+      assert(gotFirstData);
+      assert(gotFirstEnd);
+      console.log('ok');
+    }
+  }
+});