Skip to content
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

Merged
merged 9 commits into from
Feb 28, 2018
4 changes: 3 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,10 @@ profile-*
.vscode
*code-workspace

# flamegraphs
# clinic
profile*
*clinic*
*flamegraph*

# lock files
yarn.lock
Expand Down
1 change: 0 additions & 1 deletion examples/example.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ const opts = {

fastify
.get('/', opts, function (req, reply) {
reply.header('Content-Type', 'application/json').code(200)
Copy link
Member

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?

Copy link
Member Author

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.

reply.send({ hello: 'world' })
})
.get('/promise', opts, function (req, reply) {
Expand Down
6 changes: 4 additions & 2 deletions lib/handleRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the old code prevent V8 from optimizing the constructor call?

Copy link
Member Author

Choose a reason for hiding this comment

The 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)
Expand Down
101 changes: 81 additions & 20 deletions lib/reply.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ const serializeError = FJS({
}
})

var getHeader

function Reply (res, context, request) {
this.res = res
this.context = context
Expand All @@ -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) {
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the change to var here an optimization?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be, but I didn't measure without.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe change it back for consistency?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsumners as far as I know var is slightly faster than const because of the scoping.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsumners @delvedor I tried reverting the const -> var change and the context thing, and I got a slight downgrade of the throughput. I prefer to leave them as they are in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which means the answer to the question is "yes".

Copy link
Member Author

Choose a reason for hiding this comment

The 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
}
Expand All @@ -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)
}
Expand All @@ -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
}

Expand All @@ -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
Copy link
Member

@allevo allevo Feb 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.header() ?

Copy link
Member Author

Choose a reason for hiding this comment

The 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
}

Expand All @@ -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) {
Expand All @@ -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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A server MAY send a Content-Length header field in a response to a HEAD request (Section 4.3.2 of [RFC7231]); a server MUST NOT send Content-Length in such a response unless its field-value equals the decimal number of octets that would have been sent in the payload body of a response if the same request had used the GET method.

If a user sends a 200 response to a HEAD request, this will most likely be the wrong Content-Length.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except this is within the condition that the payload is either undefined or null. Thus, there isn't a content length.

Copy link
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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:

fastify/lib/reply.js

Lines 149 to 163 in 578517d

if (payload === undefined || payload === null) {
reply.sent = true
// 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'
}
res.writeHead(statusCode, reply._headers)
// avoid ArgumentsAdaptorTrampoline from V8
res.end(null, null, null)
return
}

The payload size should always be 0 where you are explicitly setting the content length because a payload does not exist.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's okay. I'm not going to block this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still appropriate to send Content-length: 0 in that case because it is true. Headers do not count.

}

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']) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 res.

Copy link
Member

@delvedor delvedor Feb 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the final handler of the onSend hooks, so if a user wants to alter the headers it will use the reply interface. I hope that no one will set the content-length header inside a onRequest or preHandler hook.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -271,6 +300,7 @@ function buildReply (R) {
this.sent = false
this._serializer = null
this.request = request
this._headers = {}
}
_Reply.prototype = new R()
return _Reply
Expand All @@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using res.hasHeader doesn't seem like it would help much. It's almost the exact same code as res.getHeader so it would be simpler just to have one function that's the same as getHeaderFallback.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not have investigated why, but res.getHeader slow us down, while res.hasHeader does not.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
54 changes: 49 additions & 5 deletions test/hooks.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,7 @@ test('onSend hook is called after payload is serialized and headers are set', t

instance.addHook('onSend', function (request, reply, payload, next) {
t.deepEqual(JSON.parse(payload), thePayload)
t.strictEqual(reply.res.getHeader('Content-Type'), 'application/json')
t.strictEqual(reply._headers['content-type'], 'application/json')
next()
})

Expand All @@ -711,7 +711,7 @@ test('onSend hook is called after payload is serialized and headers are set', t
fastify.register((instance, opts, next) => {
instance.addHook('onSend', function (request, reply, payload, next) {
t.strictEqual(payload, 'some text')
t.strictEqual(reply.res.getHeader('Content-Type'), 'text/plain')
t.strictEqual(reply._headers['content-type'], 'text/plain')
next()
})

Expand All @@ -727,7 +727,7 @@ test('onSend hook is called after payload is serialized and headers are set', t

instance.addHook('onSend', function (request, reply, payload, next) {
t.strictEqual(payload, thePayload)
t.strictEqual(reply.res.getHeader('Content-Type'), 'application/octet-stream')
t.strictEqual(reply._headers['content-type'], 'application/octet-stream')
next()
})

Expand All @@ -749,7 +749,7 @@ test('onSend hook is called after payload is serialized and headers are set', t

instance.addHook('onSend', function (request, reply, payload, next) {
t.strictEqual(payload, thePayload)
t.strictEqual(reply.res.getHeader('Content-Type'), 'application/octet-stream')
t.strictEqual(reply._headers['content-type'], 'application/octet-stream')
next()
})

Expand All @@ -765,7 +765,7 @@ test('onSend hook is called after payload is serialized and headers are set', t

instance.addHook('onSend', function (request, reply, payload, next) {
t.strictEqual(payload, serializedPayload)
t.strictEqual(reply.res.getHeader('Content-Type'), 'text/custom')
t.strictEqual(reply._headers['content-type'], 'text/custom')
next()
})

Expand Down Expand Up @@ -1675,6 +1675,50 @@ test('onRequest, preHandler, and onResponse hooks that resolve to a value do not
})
})

test('If a response header has been set inside an hook it shoulod not be overwritten by the final response handler', t => {
t.plan(5)
const fastify = Fastify()

fastify.addHook('onRequest', (req, res, next) => {
res.setHeader('X-Custom-Header', 'hello')
next()
})

fastify.get('/', (request, reply) => {
reply.send('hello')
})

fastify.inject('/', (err, res) => {
t.error(err)
t.strictEqual(res.headers['x-custom-header'], 'hello')
t.strictEqual(res.headers['content-type'], 'text/plain')
t.strictEqual(res.statusCode, 200)
t.strictEqual(res.payload, 'hello')
})
})

test('If the content type has been set inside an hook it should not be changed', t => {
t.plan(5)
const fastify = Fastify()

fastify.addHook('onRequest', (req, res, next) => {
res.setHeader('content-type', 'text/html')
next()
})

fastify.get('/', (request, reply) => {
t.notOk(reply._headers['content-type'])
reply.send('hello')
})

fastify.inject('/', (err, res) => {
t.error(err)
t.strictEqual(res.headers['content-type'], 'text/html')
t.strictEqual(res.statusCode, 200)
t.strictEqual(res.payload, 'hello')
})
})

if (Number(process.versions.node[0]) >= 8) {
require('./hooks-async')(t)
} else {
Expand Down
10 changes: 2 additions & 8 deletions test/internals/handleRequest.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,7 @@ test('handler function - invalid schema', t => {
t.equal(res.statusCode, 400)
t.pass()
}
res.setHeader = (key, value) => {
return
}
res.getHeader = (key) => {
res.writeHead = () => {
return
}
res.log = { error: () => {}, info: () => {} }
Expand Down Expand Up @@ -81,10 +78,7 @@ test('handler function - reply', t => {
t.equal(res.statusCode, 204)
t.pass()
}
res.getHeader = (key) => {
return false
}
res.setHeader = (key, value) => {
res.writeHead = () => {
return
}
const context = {
Expand Down