fix headersSent check

This issue was discovered when an over-limit request body is
sent to LB4. Destroying the request socket causing the client
to fail instead of receiving a meaningful error.
This commit is contained in:
Raymond Feng 2018-10-17 09:43:07 -07:00
parent e7aa8353e7
commit 26e8f8f3eb
3 changed files with 32 additions and 5 deletions

3
index.d.ts vendored
View File

@ -27,13 +27,14 @@ declare namespace errorHandlerFactory {
* @param req Incoming request * @param req Incoming request
* @param res Response * @param res Response
* @param options Options for error handler settings * @param options Options for error handler settings
* @returns false if the response has been sent before the error
*/ */
function writeErrorToResponse( function writeErrorToResponse(
err: Error, err: Error,
req: Express.Request, req: Express.Request,
res: Express.Response, res: Express.Response,
options?: ErrorWriterOptions options?: ErrorWriterOptions
): void; ): boolean;
/** /**
* Error-handling middleware function. Includes server-side logging * Error-handling middleware function. Includes server-side logging

View File

@ -10,7 +10,6 @@ var SG = require('strong-globalize');
SG.SetRootDir(path.resolve(__dirname, '..')); SG.SetRootDir(path.resolve(__dirname, '..'));
var buildResponseData = require('./data-builder'); var buildResponseData = require('./data-builder');
var debug = require('debug')('strong-error-handler'); var debug = require('debug')('strong-error-handler');
var format = require('util').format;
var logToConsole = require('./logger'); var logToConsole = require('./logger');
var negotiateContentProducer = require('./content-negotiation'); var negotiateContentProducer = require('./content-negotiation');
@ -50,9 +49,12 @@ function writeErrorToResponse(err, req, res, options) {
options = options || {}; options = options || {};
if (res._header) { // See https://nodejs.org/api/http.html#http_response_headerssent
debug('Response was already sent, closing the underlying connection'); if (res.headersSent) {
return req.socket.destroy(); debug('Response was already sent. Skipping error response.');
// We should not destroy the request socket as it causes Error: write EPIPE
// return req.socket.destroy();
return false;
} }
// this will alter the err object, to handle when res.statusCode is an error // this will alter the err object, to handle when res.statusCode is an error
@ -67,6 +69,7 @@ function writeErrorToResponse(err, req, res, options) {
var sendResponse = negotiateContentProducer(req, warn, options); var sendResponse = negotiateContentProducer(req, warn, options);
sendResponse(res, data); sendResponse(res, data);
return true;
function warn(msg) { function warn(msg) {
res.header('X-Warning', msg); res.header('X-Warning', msg);

View File

@ -38,6 +38,24 @@ describe('strong-error-handler', function() {
request.get('/').expect(200, 'empty', done); request.get('/').expect(200, 'empty', done);
}); });
it('handles response headers already sent without destroying the request',
function(done) {
givenErrorHandlerForError();
var handler = _requestHandler;
_requestHandler = function(req, res, next) {
res.end('empty');
process.nextTick(function() {
handler(req, res, next);
});
};
request.post('/').send(givenLargeRequest())
.expect(200, 'empty', function(err) {
// Skip EPIPE
if (err && err.code !== 'EPIPE') return done(err);
else return done();
});
});
context('status code', function() { context('status code', function() {
it('converts non-error "err.status" to 500', function(done) { it('converts non-error "err.status" to 500', function(done) {
givenErrorHandlerForError(new ErrorWithProps({status: 200})); givenErrorHandlerForError(new ErrorWithProps({status: 200}));
@ -919,3 +937,8 @@ function getExpectedErrorData(err) {
cloneAllProperties(data, err); cloneAllProperties(data, err);
return data; return data;
} }
function givenLargeRequest() {
const data = Buffer.alloc(2 * 1024 * 1024, 'A', 'utf-8');
return data.toString();
}