fix: run onError hooks after error handler

[jean-michelet]

Fixes: https://github.com/fastify/fastify/issues/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 onError hook should be called before error handlers : https://github.com/fastify/fastify/blob/52eea4b1b84ae6634c22eabf34e1e4547a330b31/test/encapsulated-error-handler.test.js#L68

But the doc says otherwise:

[jean-michelet]

We need to asses the error first

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.

[Eomm]

What do you mean?

I mean that I would like to understand:

• when was it introduced?
• why did we not spot it earlier?
• is the wanted behaviour correct?

Given that:

• the onError hook is designed to be an informative hook, where the user should not use reply.send() in this context
• the custom error handler is the function that should choose what to do with the error

So, if we fix this bug:

1. we are turning off the onError function calls that are after the custom error handler
2. other?

Given 1., are we breaking some monitoring tools or database module? (since we use in postgres, rate-limit, multipart etc... https://github.com/search?q=org%3Afastify+%22onError%22+-path%3Atest%2F+-path%3Atypes%2F+language%3AJavaScript+-repo%3Afastify%2Ffastify&type=code

so I wonder if this fix should be considered a breaking change.

I tend to agree

[gurgunday]

why did we not spot it earlier?

This is indeed interesting, I might look into this if I can find some time

[jean-michelet]

@fastify/core

Maybe we should close this PR and update the doc, wdyt?

[jean-michelet]

The issue I have is that the documentation is incorrect. This can be frustrating, as users generally trust documentations and may end up wasting time and energy due to misleading information.

At the same time, the current implementation is likely difficult to change, and as @Eomm pointed out, the hook is mainly informational.

[gurgunday]

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

[jean-michelet]

@gurgunday

Any progress in your investigation?

[gurgunday]

Unfortunately I couldn't make any progress whatsoever on this - had so little time!

Feel free to take it over

[jean-michelet]

Should we close this @fastify/core? I don't think we can/should go back