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
crash: errors thrown in a hook and then rethrown by an error handler crash fastify #5220
Comments
Confimed. This is the stack trace:
What is missing is wrapping Line 65 in e3a07ea
|
|
might be a bit too hard for that, but one can try |
I'm happy to take a stab at fixing this issue. Why is this happeningI'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: (also relevant context: Example flow (all line numbers reference handleRequest.js):
These effectively give us 2 attempts for an error handler to work, because they call Other side note: How we can fix itGiven 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 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. |
…lers Parent error handling for synchronous error handlers was largely a unintended byproduct of some other try catches previously. This could result in a server crash if multiple error handlers were used, and rethrew errors (see fastify#5220). This PR fixes this behavior by retrying synchronous error handling (similar to how we already do in wrapThenable for asynchronous error handling). It also adds regression tests to ensure this continues to work properly.
@climba03003 Can you reopen this issue? It's not actually fixed in #5445 - see #5451 |
I will consider to re-open once you answer the question in the PR you propose. |
Prerequisites
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:
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.
The text was updated successfully, but these errors were encountered: