Re-throw non-Error is not handled by parent handler

[camsteffen]

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

18.19.0

Operating system

macOS

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

14.2

Description

If a plugin error handler re-throws the error and the error is not an instance of Error, then the error does not propagate to the parent context error handler. Instead the error is simply handled by the default error handler.

I understand that throwing a non-Error is not good practice. This behavior caught me off guard when I was writing dummy code just to learn-by-doing how error handlers work. I had falsely assumed that fastify simply doesn't bubble up errors based on my test.

Steps to Reproduce

const app = require("fastify")({ logger: true });

app.setErrorHandler((error, request, reply) => {
  console.log("parent");
  reply.status(500).send({ ok: false });
});

app.register((app, opts, next) => {
  app.setErrorHandler((error, request, reply) => {
    console.log("child");
    throw error;
  });

  app.get("/bad", async () => {
    // this error skips over the parent error handler
    throw "garbage";
  });
  app.get("/good", async () => {
    throw new Error("party");
  });

  next();
});

app.listen({ port: 8080 });

Expected Behavior

The parent error handler sees the non-Error error.

[metcoder95]

👋
I can confirm I can reproduce the issue, although I'm not sure if this was the expected intention of the original design.

So far the error handling works as expected, I tried to narrow down to exactly the place that might be swallowing this error but haven't found it yet, I belive is somewhere near where we call the child error handling that we are ignoring the error thrown.

I'll try to revisit this later on.

[mcollina]

This is expected and by design. It's not something we can easily change, as it might require a significant semver major refactor and possibly a drop in performance.

[metcoder95]

Kind of imagined that, specially as it was pretty specific.

Should we document this behaviour?
It's kind of "implied" that people should be outside throwing everything that is not an error; might be worth it as a common recommendation?

[mcollina]

+1 on the documentation approach