Create a separate hook runner for onSend hooks

[nwoltman]

The onSend hooks are a special case because they need extra logic for changing the payload. This extra logic is not needed for the other hooks and it makes the other hooks prone to a bug where the hook runner's internal state will be changed if a hook function returns a promise that resolves to a value other than undefined.

Example of the bug that this change prevents

const fastify = require('./')()

fastify.addHook('preHandler', (request, reply) => {
  return Promise.resolve() // OK
})

fastify.addHook('preHandler', (request, reply) => {
  return Promise.resolve(true) // Causes crash
})

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

fastify.inject('/', (err, res) => {
  console.log('Will not reach here')
})

(node:12944) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'finished' of undefined
    at preHandlerCallback (~/fastify/lib/handleRequest.js:80:17)
    at next (~/fastify/lib/hookRunner.js:17:7)
    at handleResolve (~/fastify/lib/hookRunner.js:28:5)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:160:7)
    at Function.Module.runMain (module.js:703:11)
    at startup (bootstrap_node.js:190:16)
    at bootstrap_node.js:662:3

This change adds a second hook runner just for onSend hooks that helps make the code that starts the hooks cleaner (it's no longer necessary to call .bind(reply) during a request) and the main hook runner is simplified so that changing its internal state is no longer possible.

This also improves the hooks benchmark by 450-600 req/sec.

Benchmark

Before

[1] Stat         Avg     Stdev   Max
[1] Latency (ms) 2.58    6.92    195.71
[1] Req/Sec      35211.2 6839.11 39080
[1] Bytes/Sec    5.26 MB 1.03 MB 5.82 MB
[1]
[1] 176k requests in 5s, 26.2 MB read
...
[1] Stat         Avg     Stdev   Max
[1] Latency (ms) 2.61    6.8     165.82
[1] Req/Sec      34753.6 7256.24 39432
[1] Bytes/Sec    5.23 MB 1.08 MB 5.88 MB
[1]
[1] 174k requests in 5s, 25.9 MB read

After

[1] Stat         Avg      Stdev   Max
[1] Latency (ms) 2.57     7.03    198.39
[1] Req/Sec      35204.81 7148.29 40124
[1] Bytes/Sec    5.23 MB  1.08 MB 5.98 MB
[1]
[1] 176k requests in 5s, 26.2 MB read
...
[1] Stat         Avg      Stdev   Max
[1] Latency (ms) 2.53     6.8     190.88
[1] Req/Sec      35812.81 6979.73 39894
[1] Bytes/Sec    5.31 MB  1.05 MB 5.94 MB
[1]
[1] 179k requests in 5s, 26.7 MB read

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows Code of conduct

[mcollina]

Can you please add a test for the original bug you wanted to solve?

[nwoltman]

It's right here: https://github.com/fastify/fastify/pull/795/files#diff-810aa058de5dd12d6a4d43556a2eb5e6R178

[mcollina]

I mean one using the external API. Il giorno dom 18 feb 2018 alle 08:45 Nathan Woltman < notifications@github.com> ha scritto:

…

It's right here: https://github.com/fastify/fastify/pull/795/files#diff-810aa058de5dd12d6a4d43556a2eb5e6R178 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#795 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADL498uoKGnfqSCz-C90q4IBgyatO2Jks5tV9UQgaJpZM4SJhi5> .

[mcollina]

LGTM

[delvedor]

LGTM

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.