fix: run onError hooks after error handler #6146
Conversation
| () => { | ||
| reply[kReplyIsRunningOnErrorHook] = false | ||
| } |
There was a problem hiding this comment.
Here, we could access the error if an onError hook callback throw, but we need to check that the response hasn't already been sent.
Perhaps we could also issue a warning?
What do you mean? Also, even if we currently document this incorrectly and should consider it a bug fix, in practice, users might have implemented their application based on the current behavior (apparently present for years), so I wonder if this fix should be considered a breaking change. |
a249404 to
cce1f28
Compare
I mean that I would like to understand:
Given that:
So, if we fix this bug:
Given
I tend to agree |
This is indeed interesting, I might look into this if I can find some time |
|
@fastify/core Maybe we should close this PR and update the doc, wdyt? |
|
The issue I have is that the documentation is incorrect. At the same time, the current implementation is likely difficult to change, and as @Eomm pointed out, the hook is mainly informational. |
|
Yeah let's update the docs in any case I'll approve a PR that does that Maybe keep this one open a little longer so I can have a look |
- Update docs to reflect that onError hook runs BEFORE error handlers - Fix Reply Lifecycle diagram to show correct execution order - Update TypeScript documentation and type definitions - Resolves discrepancy between docs and actual implementation Fixes: fastify#6135 Related: fastify#6146
* docs: fix onError hook execution order documentation - Update docs to reflect that onError hook runs BEFORE error handlers - Fix Reply Lifecycle diagram to show correct execution order - Update TypeScript documentation and type definitions - Resolves discrepancy between docs and actual implementation Fixes: #6135 Related: #6146 * docs: simplify onError hook documentation text Address review feedback from jean-michelet: - Remove redundant explanations about hook purpose - Simplify execution order descriptions - Make documentation more concise while preserving key information
|
Any progress in your investigation? |
|
Unfortunately I couldn't make any progress whatsoever on this - had so little time! Feel free to take it over |
|
Should we close this @fastify/core? |
Fixes: #6135
I'd like some feedback on this, as I'm not sure we understand how we should handle errors.
There's this test that documents that the
onErrorhook should be called before error handlers :fastify/test/encapsulated-error-handler.test.js
Line 68 in 52eea4b
But the doc says otherwise:
