-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix the headersSent check and avoid Error: write EPIPE #79
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,6 @@ var SG = require('strong-globalize'); | |
SG.SetRootDir(path.resolve(__dirname, '..')); | ||
var buildResponseData = require('./data-builder'); | ||
var debug = require('debug')('strong-error-handler'); | ||
var format = require('util').format; | ||
var logToConsole = require('./logger'); | ||
var negotiateContentProducer = require('./content-negotiation'); | ||
|
||
|
@@ -50,9 +49,12 @@ function writeErrorToResponse(err, req, res, options) { | |
|
||
options = options || {}; | ||
|
||
if (res._header) { | ||
debug('Response was already sent, closing the underlying connection'); | ||
return req.socket.destroy(); | ||
// See https://nodejs.org/api/http.html#http_response_headerssent | ||
if (res.headersSent) { | ||
debug('Response was already sent. Skipping error response.'); | ||
// We should not destroy the request socket as it causes Error: write EPIPE | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For me, the EPIPE error is still happening without destroying the request socket. Do we want to throw a meaningful error here instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. The EPIPE error is reported from the client side as the socket is destroyed by the server. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, any idea on why it's failing for me on node 10?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A bit more verbose logs:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see it now. It's interesting, |
||
// return req.socket.destroy(); | ||
return false; | ||
} | ||
|
||
// 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); | ||
sendResponse(res, data); | ||
return true; | ||
|
||
function warn(msg) { | ||
res.header('X-Warning', msg); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,24 @@ describe('strong-error-handler', function() { | |
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am afraid this test does not fail with the old implementation either. I am concerned that this test may never fail, which would give us a false sense of security. |
||
else return done(); | ||
}); | ||
}); | ||
|
||
context('status code', function() { | ||
it('converts non-error "err.status" to 500', function(done) { | ||
givenErrorHandlerForError(new ErrorWithProps({status: 200})); | ||
|
@@ -919,3 +937,8 @@ function getExpectedErrorData(err) { | |
cloneAllProperties(data, err); | ||
return data; | ||
} | ||
|
||
function givenLargeRequest() { | ||
const data = Buffer.alloc(2 * 1024 * 1024, 'A', 'utf-8'); | ||
return data.toString(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're removing the case for
res._header
because it no longer exists and replaced byheadersSent
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
headersSent
is the documented api while_header
is an internal property.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to use
res.headersSent