-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Use writeHead instead of setHeader/getHeader. #813
Changes from all commits
d4a5c20
5d18bde
f4a5f18
cf016db
b7c74f5
44b520a
3385b33
163f98f
578517d
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 |
---|---|---|
|
@@ -13,8 +13,10 @@ const inputSchemaError = fastJsonStringify(schemas.inputSchemaError) | |
function handleRequest (req, res, params, context) { | ||
var method = req.method | ||
var headers = req.headers | ||
var request = new context.Request(params, req, urlUtil.parse(req.url, true).query, headers, req.log) | ||
var reply = new context.Reply(res, context, request) | ||
var Request = context.Request | ||
var Reply = context.Reply | ||
var request = new Request(params, req, urlUtil.parse(req.url, true).query, headers, req.log) | ||
var reply = new Reply(res, context, request) | ||
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. Does the old code prevent V8 from optimizing the constructor call? 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. not really, but I had to fix a bad bug in my change and I forgot these in. I can revert this bit if you want. |
||
|
||
if (method === 'GET' || method === 'HEAD') { | ||
handler(reply) | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -19,6 +19,8 @@ const serializeError = FJS({ | |||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
var getHeader | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
function Reply (res, context, request) { | ||||||||||||||||||||||||||||||||
this.res = res | ||||||||||||||||||||||||||||||||
this.context = context | ||||||||||||||||||||||||||||||||
|
@@ -27,6 +29,7 @@ function Reply (res, context, request) { | |||||||||||||||||||||||||||||||
this._customError = false | ||||||||||||||||||||||||||||||||
this._isError = false | ||||||||||||||||||||||||||||||||
this.request = request | ||||||||||||||||||||||||||||||||
this._headers = {} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
Reply.prototype.send = function (payload) { | ||||||||||||||||||||||||||||||||
|
@@ -47,20 +50,20 @@ Reply.prototype.send = function (payload) { | |||||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
const contentType = this.res.getHeader('Content-Type') | ||||||||||||||||||||||||||||||||
const contentTypeNotSet = contentType === undefined | ||||||||||||||||||||||||||||||||
var contentType = getHeader(this, 'content-type') | ||||||||||||||||||||||||||||||||
var contentTypeNotSet = contentType === undefined | ||||||||||||||||||||||||||||||||
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. Is the change to 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. It might be, but I didn't measure without. 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. Maybe change it back for consistency? 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. @jsumners as far as I know 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. 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. Which means the answer to the question is "yes". 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. yes. |
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
if (payload !== null) { | ||||||||||||||||||||||||||||||||
if (Buffer.isBuffer(payload) || typeof payload.pipe === 'function') { | ||||||||||||||||||||||||||||||||
if (contentTypeNotSet) { | ||||||||||||||||||||||||||||||||
this.res.setHeader('Content-Type', 'application/octet-stream') | ||||||||||||||||||||||||||||||||
this._headers['content-type'] = 'application/octet-stream' | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
onSendHook(this, payload) | ||||||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
if (contentTypeNotSet && typeof payload === 'string') { | ||||||||||||||||||||||||||||||||
this.res.setHeader('Content-Type', 'text/plain') | ||||||||||||||||||||||||||||||||
this._headers['content-type'] = 'text/plain' | ||||||||||||||||||||||||||||||||
onSendHook(this, payload) | ||||||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
@@ -69,7 +72,7 @@ Reply.prototype.send = function (payload) { | |||||||||||||||||||||||||||||||
if (this._serializer) { | ||||||||||||||||||||||||||||||||
payload = this._serializer(payload) | ||||||||||||||||||||||||||||||||
} else if (contentTypeNotSet || contentType === 'application/json') { | ||||||||||||||||||||||||||||||||
this.res.setHeader('Content-Type', 'application/json') | ||||||||||||||||||||||||||||||||
this._headers['content-type'] = 'application/json' | ||||||||||||||||||||||||||||||||
payload = serialize(this.context, payload, this.res.statusCode) | ||||||||||||||||||||||||||||||||
flatstr(payload) | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
@@ -78,7 +81,7 @@ Reply.prototype.send = function (payload) { | |||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
Reply.prototype.header = function (key, value) { | ||||||||||||||||||||||||||||||||
this.res.setHeader(key, value) | ||||||||||||||||||||||||||||||||
this._headers[key.toLowerCase()] = value | ||||||||||||||||||||||||||||||||
return this | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
|
@@ -105,7 +108,7 @@ Reply.prototype.serializer = function (fn) { | |||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
Reply.prototype.type = function (type) { | ||||||||||||||||||||||||||||||||
this.res.setHeader('Content-Type', type) | ||||||||||||||||||||||||||||||||
this._headers['content-type'] = type | ||||||||||||||||||||||||||||||||
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.
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. why going for a slow-path when there is a fast-path? |
||||||||||||||||||||||||||||||||
return this | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
|
@@ -115,7 +118,7 @@ Reply.prototype.redirect = function (code, url) { | |||||||||||||||||||||||||||||||
code = 302 | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
this.header('Location', url).code(code).send() | ||||||||||||||||||||||||||||||||
this.header('location', url).code(code).send() | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
function onSendHook (reply, payload) { | ||||||||||||||||||||||||||||||||
|
@@ -140,27 +143,44 @@ function wrapOnSendEnd (err, reply, payload) { | |||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
function onSendEnd (reply, payload) { | ||||||||||||||||||||||||||||||||
var res = reply.res | ||||||||||||||||||||||||||||||||
var statusCode = res.statusCode | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
if (payload === undefined || payload === null) { | ||||||||||||||||||||||||||||||||
reply.sent = true | ||||||||||||||||||||||||||||||||
reply.res.end() | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
// according to https://tools.ietf.org/html/rfc7230#section-3.3.2 | ||||||||||||||||||||||||||||||||
// we cannot send a content-length for 304 and 204, and all status code | ||||||||||||||||||||||||||||||||
// < 200. | ||||||||||||||||||||||||||||||||
if (statusCode >= 200 && statusCode !== 204 && statusCode !== 304) { | ||||||||||||||||||||||||||||||||
reply._headers['content-length'] = '0' | ||||||||||||||||||||||||||||||||
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.
If a user sends a 200 response to a HEAD request, this will most likely be the wrong Content-Length. 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. Except this is within the condition that the payload is either 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. what should it be in that case? Should we check if there is a content-length set before setting it to zero? 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’m completely lost. This code passes all our tests in master without changes. 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. @mcollina I believe you are correct with the code as it is. Look at the full context: Lines 149 to 163 in 578517d
The payload size should always be 0 where you are explicitly setting the content length because a payload does not exist. 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. @jsumners If it's a HEAD request, the user may decide to only set the headers and not send the payload itself (even though it would be stripped by Node). Users can do this to avoid spending time generating a payload. @mcollina In that case the Content-Length should not be set if it was not already set by a user. 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. It's okay. I'm not going to block this. 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. It's still appropriate to send |
||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
res.writeHead(statusCode, reply._headers) | ||||||||||||||||||||||||||||||||
// avoid ArgumentsAdaptorTrampoline from V8 | ||||||||||||||||||||||||||||||||
res.end(null, null, null) | ||||||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
if (typeof payload.pipe === 'function') { | ||||||||||||||||||||||||||||||||
sendStream(payload, reply.res, reply) | ||||||||||||||||||||||||||||||||
sendStream(payload, res, reply) | ||||||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
if (typeof payload !== 'string' && !Buffer.isBuffer(payload)) { | ||||||||||||||||||||||||||||||||
throw new TypeError(`Attempted to send payload of invalid type '${typeof payload}'. Expected a string or Buffer.`) | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
if (!reply.res.getHeader('Content-Length')) { | ||||||||||||||||||||||||||||||||
reply.res.setHeader('Content-Length', '' + Buffer.byteLength(payload)) | ||||||||||||||||||||||||||||||||
if (!reply._headers['content-length']) { | ||||||||||||||||||||||||||||||||
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. Same thing as my other comment about the header possibly being set on 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. This is the final handler of the 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 also hope that no one will do that, but we can't stop them from doing it so it's a case that needs to be handled. 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 don't agree, I don't think we should drop a considerable amount of ops/sec just because a user may do something that is wrong. |
||||||||||||||||||||||||||||||||
reply._headers['content-length'] = '' + Buffer.byteLength(payload) | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
reply.sent = true | ||||||||||||||||||||||||||||||||
reply.res.end(payload) | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
res.writeHead(statusCode, reply._headers) | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
// avoid ArgumentsAdaptorTrampoline from V8 | ||||||||||||||||||||||||||||||||
res.end(payload, null, null) | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
function sendStream (payload, res, reply) { | ||||||||||||||||||||||||||||||||
|
@@ -196,11 +216,19 @@ function sendStream (payload, res, reply) { | |||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
// streams will error asynchronously, and we want to handle that error | ||||||||||||||||||||||||||||||||
// appropriately, e.g. a 404 for a missing file. So we cannot use | ||||||||||||||||||||||||||||||||
// writeHead, and we need to resort to setHeader, which will trigger | ||||||||||||||||||||||||||||||||
// a writeHead when there is data to send. | ||||||||||||||||||||||||||||||||
for (var key in reply._headers) { | ||||||||||||||||||||||||||||||||
res.setHeader(key, reply._headers[key]) | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
payload.pipe(res) | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
function handleError (reply, error, cb) { | ||||||||||||||||||||||||||||||||
var statusCode = reply.res.statusCode | ||||||||||||||||||||||||||||||||
var res = reply.res | ||||||||||||||||||||||||||||||||
var statusCode = res.statusCode | ||||||||||||||||||||||||||||||||
statusCode = (statusCode >= 400) ? statusCode : 500 | ||||||||||||||||||||||||||||||||
if (error != null) { | ||||||||||||||||||||||||||||||||
if (error.status >= 400) { | ||||||||||||||||||||||||||||||||
|
@@ -218,12 +246,12 @@ function handleError (reply, error, cb) { | |||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
reply.res.statusCode = statusCode | ||||||||||||||||||||||||||||||||
res.statusCode = statusCode | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
if (statusCode >= 500) { | ||||||||||||||||||||||||||||||||
reply.res.log.error({ req: reply.request.raw, res: reply.res, err: error }, error && error.message) | ||||||||||||||||||||||||||||||||
res.log.error({ req: reply.request.raw, res: res, err: error }, error && error.message) | ||||||||||||||||||||||||||||||||
} else if (statusCode >= 400) { | ||||||||||||||||||||||||||||||||
reply.res.log.info({ res: reply.res, err: error }, error && error.message) | ||||||||||||||||||||||||||||||||
res.log.info({ res: res, err: error }, error && error.message) | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
if (error && error.headers) { | ||||||||||||||||||||||||||||||||
|
@@ -250,16 +278,17 @@ function handleError (reply, error, cb) { | |||||||||||||||||||||||||||||||
statusCode: statusCode | ||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||
flatstr(payload) | ||||||||||||||||||||||||||||||||
reply.res.setHeader('Content-Type', 'application/json') | ||||||||||||||||||||||||||||||||
reply._headers['content-type'] = 'application/json' | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
if (cb) { | ||||||||||||||||||||||||||||||||
cb(reply, payload) | ||||||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
reply.res.setHeader('Content-Length', '' + Buffer.byteLength(payload)) | ||||||||||||||||||||||||||||||||
reply._headers['content-length'] = '' + Buffer.byteLength(payload) | ||||||||||||||||||||||||||||||||
reply.sent = true | ||||||||||||||||||||||||||||||||
reply.res.end(payload) | ||||||||||||||||||||||||||||||||
res.writeHead(res.statusCode, reply._headers) | ||||||||||||||||||||||||||||||||
res.end(payload) | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
function buildReply (R) { | ||||||||||||||||||||||||||||||||
|
@@ -271,6 +300,7 @@ function buildReply (R) { | |||||||||||||||||||||||||||||||
this.sent = false | ||||||||||||||||||||||||||||||||
this._serializer = null | ||||||||||||||||||||||||||||||||
this.request = request | ||||||||||||||||||||||||||||||||
this._headers = {} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
_Reply.prototype = new R() | ||||||||||||||||||||||||||||||||
return _Reply | ||||||||||||||||||||||||||||||||
|
@@ -292,5 +322,36 @@ function notFound (reply) { | |||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
function noop () {} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
function getHeaderProper (reply, key) { | ||||||||||||||||||||||||||||||||
key = key.toLowerCase() | ||||||||||||||||||||||||||||||||
var res = reply.res | ||||||||||||||||||||||||||||||||
var value = reply._headers[key] | ||||||||||||||||||||||||||||||||
if (value === undefined && res.hasHeader(key)) { | ||||||||||||||||||||||||||||||||
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. Using 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 do not have investigated why, but 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. Oh that is very interesting. |
||||||||||||||||||||||||||||||||
value = res.getHeader(key) | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
return value | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
function getHeaderFallback (reply, key) { | ||||||||||||||||||||||||||||||||
key = key.toLowerCase() | ||||||||||||||||||||||||||||||||
var res = reply.res | ||||||||||||||||||||||||||||||||
var value = reply._headers[key] | ||||||||||||||||||||||||||||||||
if (value === undefined) { | ||||||||||||||||||||||||||||||||
value = res.getHeader(key) | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
return value | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
// ponyfill for hasHeader. It has been intoroduced into Node 7.7, | ||||||||||||||||||||||||||||||||
// so it's ok to use it in 8+ | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
const v = process.version.match(/v(\d+)/)[1] | ||||||||||||||||||||||||||||||||
if (Number(v) > 7) { | ||||||||||||||||||||||||||||||||
getHeader = getHeaderProper | ||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||
getHeader = getHeaderFallback | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
module.exports = Reply | ||||||||||||||||||||||||||||||||
module.exports.buildReply = buildReply |
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.
Why this line is removed?
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.
it was not needed, those are the defaults.