From c9dcf5718cf47322b00f2994797aebb68d7ed0fb Mon Sep 17 00:00:00 2001 From: isaacs Date: Thu, 14 Feb 2013 08:57:32 -0800 Subject: [PATCH] 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 --- lib/http.js | 21 ++- .../test-http-destroyed-socket-write.js | 131 ++++++++++++++++++ 2 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 test/simple/test-http-destroyed-socket-write.js diff --git a/lib/http.js b/lib/http.js index 315a9c6a249..6f3eff1ba8c 100644 --- 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 index 00000000000..c5629871018 --- /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'); + } + } +});