improve setErrorHandler example

[trim21]

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

The example in document will make fastify never send a response if handle throw a error which is not expected error. For example if a handle throw new Error(...). Original document is not clear about this and example doesn't handle it.

Original document make me confused, I assumed that rest of errors will be handled automatically without returning them, but turns out it doesn't. I'm not sure if "error handler return unhanded error and pop them to parent handler" it expected behavior, but looks like it is, so I create this pr to fix them example. And if it's, I hope the document of setErrorHandler could be updated to o, so users can know they can pop error to default error handler. https://www.fastify.io/docs/latest/Reference/Server/#seterrorhandler

like this: GET /bad-case

fastify.get('/', function(request, reply) {
  reply.code('bad status code').send({ hello: 'world' })
});

fastify.get('/bad-case', function(request, reply) {
  throw new Error("");
});


fastify.setErrorHandler(function(error, request, reply) {
  if (error instanceof Fastify.errorCodes.FST_ERR_BAD_STATUS_CODE) {
    // Log error
    this.log.error(error);
    // Send error response
    reply.status(500).send({ ok: false });
  }
});

[mcollina]

lgtm

[trim21]

@Fdawgs your suggestions doesn't show

[Fdawgs]

@Fdawgs your suggestions doesn't show

Yeah something was up when doing it on mobile.
Will review properly next week when back from leave and at a computer.

[trim21]

@Fdawgs your suggestions doesn't show

Yeah something was up when doing it on mobile. Will review properly next week when back from leave and at a computer.

I find your suggestions in notification email

[trim21]

@Fdawgs you suggest to replace return error with throw error, but I don't think it's a good idea.

If user error handler return error, fastify will use default error handler to handle it and send a response, but if it throw error, fastify will exit and throw this error, this is documented behavior in onResponse about circular error.

[climba03003]

return error and throw error should be the same in custom error handler.

[trim21]

return error and throw error should be the same in custom error handler.

No, maybe they should, but they don't.

Try this:

main.mjs

import Fastify from 'fastify';
import * as http from 'http';

const fastify = Fastify();

fastify.get('/', async (request, reply) => {
  throw new Error("tt");
});


fastify.setErrorHandler(function(error, request, reply) {
  throw error;
});

console.log(await fastify.listen({ port: 4000, host: '127.0.0.1' }));

http.request('http://127.0.0.1:4000');

(latest fastify version, 4.10.2)

But it works with return error

[climba03003]

Interesting, but given as the example itself. It does work.

import Fastify from 'fastify';

const fastify = Fastify({ logger: false })

fastify.get('/', function(request, reply) {
  reply.code('bad status code').send({ hello: 'world' })
});

fastify.get('/bad-case', function(request, reply) {
  throw new Error("bad-case");
});


fastify.setErrorHandler(function(error, request, reply) {
  if (error instanceof Fastify.errorCodes.FST_ERR_BAD_STATUS_CODE) {
    // Send error response
    reply.status(500).send({ ok: false });
  } else {
    throw error
  }
});

{
  const { statusCode, payload } = await fastify.inject('/')
  console.log(statusCode, payload)
}

{
  const { statusCode, payload } = await fastify.inject('/bad-case')
  console.log(statusCode, payload)
}

[trim21]

@climba03003 you need to use async handler

[trim21]

- fastify.get('/bad-case', function(request, reply) {
+ fastify.get('/bad-case', async function(request, reply) {
  throw new Error("bad-case");
});

[trim21]

this will trigger node:internal/process/promises:288 triggerUncaughtException(err, true /* fromPromise */);

node:internal/process/promises:288
            triggerUncaughtException(err, true /* fromPromise */);
            ^

Error: err
    at Object.<anonymous> (c:\Users\Trim21\proj\bangumi\graphql\lib\server.ts:66:11)
    at preHandlerCallback (c:\Users\Trim21\proj\bangumi\graphql\node_modules\fastify\lib\handleRequest.js:128:37)
    at preValidationCallback (c:\Users\Trim21\proj\bangumi\graphql\node_modules\fastify\lib\handleRequest.js:112:5)
    at handler (c:\Users\Trim21\proj\bangumi\graphql\node_modules\fastify\lib\handleRequest.js:76:7)
    at handleRequest (c:\Users\Trim21\proj\bangumi\graphql\node_modules\fastify\lib\handleRequest.js:24:5)
    at runPreParsing (c:\Users\Trim21\proj\bangumi\graphql\node_modules\fastify\lib\route.js:528:5)
    at Object.routeHandler (c:\Users\Trim21\proj\bangumi\graphql\node_modules\fastify\lib\route.js:475:7)
    at Router.callHandler (c:\Users\Trim21\proj\bangumi\graphql\node_modules\find-my-way\index.js:413:14)
    at Router.lookup (c:\Users\Trim21\proj\bangumi\graphql\node_modules\find-my-way\index.js:391:17)
    at Server.preRouting (c:\Users\Trim21\proj\bangumi\graphql\node_modules\fastify\fastify.js:773:14)

[trim21]

Interesting, but given as the example itself. It does work.

This may be a bug?

[climba03003]

This may be a bug?

I would say yes, it just fall into some edge case that can actually crash the problem.
It creates an error that never able to be catch by the users.

Let see what the others say. @fastify/core

[trim21]

them I think it should be return error in this PR, considering throwing error has potential bug. 🤔

[climba03003]

LGTM.

[trim21]

About this bug, you can throw error from both sync/async handler in async error Handler, this is expected behavior

This works find in all case

fastify.setErrorHandler(async function(error, request, reply) {
  if (error instanceof Fastify.errorCodes.FST_ERR_BAD_STATUS_CODE) {
    // Send error response
    return reply.status(500).send({ ok: false });
  }

  throw error;
});

But you can't throw error from async handler in sync error handler

BUG:

fastify.get('/bad-case', async function(request, reply) {
  throw new Error("bad-case");
});


fastify.setErrorHandler(function(error, request, reply) {
  if (error instanceof Fastify.errorCodes.FST_ERR_BAD_STATUS_CODE) {
    // Send error response
    return reply.status(500).send({ ok: false });
  }

  throw error;
});

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.