crash: errors thrown in a hook and then rethrown by an error handler crash fastify

[domdomegg]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.25.1

Plugin version

No response

Node.js version

21.2.0

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

14

Description

With the following setup:

- errorHandler (outer)
- register plugin:
  - errorHandler that throws (inner)
  - hook that throws

When the hook executes, the server crashes.

Steps to Reproduce

https://github.com/domdomegg/fastify-crash-mre

Expected Behavior

The server calls the hook, then the inner errorHandler, then the errorHandler.

However, it actually calls the hook, then the inner errorHandler, then crashes.

[mcollina]

Confimed. This is the stack trace:

{"level":30,"time":1703009692048,"pid":82982,"hostname":"mcl","msg":"Server listening at http://[::1]:3000"}
{"level":30,"time":1703009692049,"pid":82982,"hostname":"mcl","msg":"Server listening at http://127.0.0.1:3000"}
{"level":30,"time":1703009705237,"pid":82982,"hostname":"mcl","reqId":"req-1","req":{"method":"GET","url":"/hook-throw-inner-throw","hostname":"localhost:3000","remoteAddress":"::1","remotePort":55846},"msg":"incoming request"}
{"level":50,"time":1703009705238,"pid":82982,"hostname":"mcl","msg":"Inner error handler got error"}
/Users/matteo/tmp/fastify-crash-mre/index.js:15
                throw new Error('Error thrown by inner handler')
                      ^

Error: Error thrown by inner handler
    at Object.<anonymous> (/Users/matteo/tmp/fastify-crash-mre/index.js:15:23)
    at handleError (/Users/matteo/tmp/fastify-crash-mre/node_modules/fastify/lib/error-handler.js:65:18)
    at onErrorHook (/Users/matteo/tmp/fastify-crash-mre/node_modules/fastify/lib/reply.js:777:5)
    at Reply.send (/Users/matteo/tmp/fastify-crash-mre/node_modules/fastify/lib/reply.js:145:5)
    at runPreParsing (/Users/matteo/tmp/fastify-crash-mre/node_modules/fastify/lib/route.js:584:11)
    at handleReject (/Users/matteo/tmp/fastify-crash-mre/node_modules/fastify/lib/hooks.js:262:7)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Node.js v20.10.0

What is missing is wrapping func in a try/catch in

fastify/lib/error-handler.js

Line 65 in e3a07ea

const result = func(error, reply.request, reply)

.

[metcoder95]

good-first-issue?

[mcollina]

might be a bit too hard for that, but one can try

[domdomegg]

I'm happy to take a stab at fixing this issue.

Why is this happening

I've spent a while looking at the internals and I managed to come up with an even simpler MRE:

const instance = require('fastify')({ logger: true })

instance.setErrorHandler(() => { throw new Error('Handler 2') })
instance.register(async (instance) => {
    instance.setErrorHandler(() => { throw new Error('Handler 1') })
    instance.get('/error', () => { throw new Error('Route error') })
})

instance.listen({ port: 3000 })

I think the reason parent error handling works at all is because we have two try catches:

• fastify/lib/handleRequest.js

  Line 77 in e3a07ea

  } catch (err) {

• fastify/lib/handleRequest.js

  Line 138 in e3a07ea

  } catch (err) {

(also relevant context: reply.send updates the context so we know which error handlers we've tried, and will try the next one, ending with the default handler. It currently does not go through all error handlers.)

Example flow (all line numbers reference handleRequest.js):

• We enter via L75 and then to L137
• We call the route handler, which throws
• We catch this on L138, and call reply.send(err). Internally uses the first error handler, which throws again. This error bubbles all the way back up and we jump back to the catch block on L77
• We enter via L78 and this time call reply.send(err) again but via L130. Internally uses the second error handler, which throws again. This error bubbles all the way back up and crashes fastify.

These effectively give us 2 attempts for an error handler to work, because they call reply.send(err) up to 2 extra times. However, when there are three errors (or we have two errors and are in the onBefore hook, which has one fewer extra retry), we run out of retries so the original error is thrown.

Other side note: reply.send's behaviour for async error handling functions is different. Here, because of wrapThenable in error-handler.js L68 it'll actually call reply.send itself again. So if we use async error handling functions everywhere things work again.

How we can fix it

Given async functions have been working this way already via wrapThenable, I am reasonably confident we can (as suggested by @mcollina) wrap func with a try catch and call reply.send(err) again. This should not cause loops given we always increment the kReplyNextErrorHandler, and terminate on the fallback.

I also considered retrying further up in handleRequest.js, but then this would not cover things like hooks etc., and means the underlying behaviour still looks different for sync and async error handlers with calls to reply.send.

I plan to submit a PR for this tomorrow, unless people have objections / other key comments.

[domdomegg]

@climba03003 Can you reopen this issue? It's not actually fixed in #5445 - see #5451

[climba03003]

I will consider to re-open once you answer the question in the PR you propose.
The only error in the PR is about the t.plan asserting wrong number.
By looking the in test, it is true that the number is wrong.