Inconsistencies with error handling

[sergejostir]

🐛 Bug Report

I noticed that errors are handled very inconsitently.

To Reproduce

Steps to reproduce the behavior:

1. Reply with "{error: true}" is sent.

const f = fastify({ logger: true });

f.setErrorHandler((error, request, reply) => {
  reply.send({ error: true });
});

f.get('/test', (request, reply) => {
  throw new Error('error 1');
});

2. Serialized "error 2" is sent.

const f = fastify({ logger: true });

f.setErrorHandler((error, request, reply) => {
  throw new Error('error 2');
});

f.get('/test', (request, reply) => {
  throw new Error('error 1');
});

3. Reply with "{error: true}" is sent. Error 2 is swallowed without notice.

const f = fastify({ logger: true });

f.setErrorHandler((error, request, reply) => {
  reply.send({ error: true });

  throw new Error('error 2');
});

f.get('/test', (request, reply) => {
  throw new Error('error 1');
});

4. Reply with "{error: true}" is sent. "Promise errored, but reply.sent = true was set" is logged.

const f = fastify({ logger: true });

f.setErrorHandler(async (error, request, reply) => {
  reply.send({ error: true });

  throw new Error('error 2');
});

f.get('/test', (request, reply) => {
  throw new Error('error 1');
});

---

In the following section we can see that throwing an error in the error handler is not supported when the handler is async, but the error handler is not, even thought I would understand as it is by reading the documentation (https://www.fastify.io/docs/latest/Errors/). On the other hand, if the error handler is async, it works properly (regardless of handler being sync or async).

5. No reply is sent. "UnhandledPromiseRejectionWarning: Error: error 2" is triggered.

const f = fastify({ logger: true });

f.setErrorHandler((error, request, reply) => {
  throw new Error('error 2');
});

f.get('/test', async (request, reply) => {
  throw new Error('error 1');
});

6. Reply with "{error: true}" is sent. "UnhandledPromiseRejectionWarning: Error: error 2" is triggered.

f.setErrorHandler((error, request, reply) => {
  reply.send({ error: true });

  throw new Error('error 2');
});

f.get('/test', async (request, reply) => {
  throw new Error('error 1');
});

7. Serialized "error 2" is sent.

const f = fastify({ logger: true });

f.setErrorHandler(async (error, request, reply) => {
  throw new Error('error 2');
});

f.get('/test', (request, reply) => {
  throw new Error('error 1');
});

---

In the following section we can see that in a sync handler only instanceof Error is regarded as an error, while in async handler everything that is thrown is regarded as an error.

fastify/lib/handleRequest.js

Lines 123 to 128 in 95bb213

	try {
	result = reply.context.handler(request, reply)
	} catch (err) {
	reply.send(err)
	return
	}

fastify/lib/wrapThenable.js

Lines 32 to 42 in 95bb213

	}, function (err) {
	if (reply[kReplySentOverwritten] === true || reply.sent === true) {
	reply.log.error({ err }, 'Promise errored, but reply.sent = true was set')
	return
	}
	reply[kReplySent] = false
	reply[kReplyIsError] = true
	reply.send(err)
	})
	}

8. Reply with "error 1" is sent. Error handler is not triggered.

const f = fastify({ logger: true });

f.get('/test', (request, reply) => {
  throw 'error 1';
});

9. Reply with "{error: true}" is sent.

const f = fastify({ logger: true });

f.setErrorHandler((error, request, reply) => {
  reply.send({ error: true });
});

f.get('/test', async (request, reply) => {
  throw 'error 1';
});

Expected behavior

In my opinion error handler should not handle errors that were thrown from within it since it's purpose is to handle errors that are sent to the handler and only a programmer error can happen within the handler itself (which cannot be handled). Therefore error (or rejection) of error handler should be propagated up the stack. I will open a new issue with an idea how to handle all these cases without introducing breaking changes. If I stick with the current state that fastify is in:

1. If an error is thrown in an error handler reply with that error serialized is sent.
2. If an error is thrown in an error handler after a reply has already been sent, let that error propagate up the stack because we are already out of fastify's scope. Alternatively fastify's FST_ERR_REP_ALREADY_SENT could be thrown, but it should include the cause (verror style).
3. Whatever is thrown in handler (regardless of it being sync or async) should trigger error handler.

Your Environment

• node version: 14
• fastify version: 3.12.0
• os: Linux

[mcollina]

From my read of this bug, it seems that throwing inside the error handler is not working as expected. I have a rough feeling we have not even designed things for this use case, and this is the result of several other fixes.

It seems the clear example of something that we have not covered is the following:

const f = fastify({ logger: true });

f.setErrorHandler((error, request, reply) => {
  reply.send({ error: true });

  throw new Error('error 2');
});

f.get('/test', (request, reply) => {
  throw new Error('error 1');
});

In the issue you write:

If an error is thrown in an error handler after a reply has already been sent, let that error propagate up the stack because we are already out of fastify's scope.

What do you mean by propagating up the stack? Could you send a PR for implementing this behavior? I think this is the most important change.

---

I would also note that throwing anything that we consider anything that is not error an antipattern. I'm not sure we should support this explicitly.

[Eomm]

As side note: I'm not sure this path has been tested in the lifecycle error flow: https://www.fastify.io/docs/latest/Lifecycle/

[Prinzhorn]

Since I've closed #2932 I'll reply here, since this is basically what @jsumners said as well:

I would also note that throwing anything that we consider anything that is not error an antipattern. I'm not sure we should support this explicitly.

I absolutely agree that this is an antipattern. But sometimes there are things not in your control and it's something that the language unfortunately allows. I personally think silently treating throw x as return x is the wrong approach to advocate people about not throwing non-Errors (and this already is very explicit support for this case, not supporting this would mean throwing a non-Error would result in an uncaughtException). This might cause incredibly hard to find bugs because you neither have an error in your server logs nor elevated 500 rates. Instead you send completely unrelated data to your client. Yes I know you can also define a schema, but sometimes things just go wrong.

I think there should at least be consistency between throw x and reject(x) where x is not an Error. Currently if you reject a promise with something that is not an error you will end up with a 500 response and the body is the rejection reason. So it behaves like a third option, sth. like rejelve(x) 😄 . This will at least give you elevated 500 rates, but unfortunately with no corresponding error log.

As long as the language allows this and sources like MDN have this

For debugging purposes and selective error catching, it is useful to make reason an instanceof Error.

we absolutely need to support this in my opinion.

'use strict';

const fastify = require('fastify')({
  logger: true,
});

fastify.get('/sync-error', (request, reply) => {
  throw new Error('/sync-error');
});

fastify.get('/sync-object', (request, reply) => {
  throw { error: '/sync-object' };
});

fastify.get('/async-error', async (request, reply) => {
  await new Promise((resolve, reject) => {
    setImmediate(() => {
      reject(new Error('/async-error'));
    });
  });
});

fastify.get('/async-object', async (request, reply) => {
  await new Promise((resolve, reject) => {
    setImmediate(() => {
      reject({ path: '/async-object' });
    });
  });
});

fastify.listen(3000, (err) => {
  if (err) {
    fastify.log.error(err);
    process.exit(1);
  }
});

curl --include http://localhost:3000/sync-error

HTTP/1.1 500 Internal Server Error
content-length: 74
content-type: application/json; charset=utf-8
Date: Sat, 13 Mar 2021 08:43:55 GMT
Connection: keep-alive
Keep-Alive: timeout=5

{"statusCode":500,"error":"Internal Server Error","message":"/sync-error"}



curl --include http://localhost:3000/sync-object

HTTP/1.1 200 OK
content-type: application/json; charset=utf-8
content-length: 24
Date: Sat, 13 Mar 2021 08:44:10 GMT
Connection: keep-alive
Keep-Alive: timeout=5

{"error":"/sync-object"}



curl --include http://localhost:3000/async-error

HTTP/1.1 500 Internal Server Error
content-length: 75
content-type: application/json; charset=utf-8
Date: Sat, 13 Mar 2021 08:44:30 GMT
Connection: keep-alive
Keep-Alive: timeout=5

{"statusCode":500,"error":"Internal Server Error","message":"/async-error"}



curl --include http://localhost:3000/async-object

HTTP/1.1 500 Internal Server Error
content-length: 24
content-type: application/json; charset=utf-8
Date: Sat, 13 Mar 2021 08:44:42 GMT
Connection: keep-alive
Keep-Alive: timeout=5

{"path":"/async-object"}

[mcollina]

I'll think through this and work on it for the next major. This is something we should resolve before we cut v4.

[Prinzhorn]

But sometimes there are things not in your control

To reiterate on that: I'm using piscina inside a request handler and when the worker throws something that is a sublcass of Error it ends up as a plain object thanks to the structured clone algorithm (nodejs/node#37988). So "You shouldn't throw anything that isn't an Error." doesn't help in that case. The worker throws an instanceof Error but I catch an object and fastify does funny things with it. You might end up sending internal information to the client. This could borderline be considered a security issue.

[sergejostir]

What do you mean by propagating up the stack? Could you send a PR for implementing this behavior? I think this is the most important change.

I meant that if anything happens that is not supposed to (for example error thrown in error handler after the reply was already sent), the error should be let go its own way (eventually reaching unhandled exception/rejection) and not being swallowed by fastify.

I already tried to investigate why this happens, but I find it a bit hard to navigate around without any comments and with that many different properties being used in every step. I will take a look again.

[RafaelGSS]

Could we close it since #3200 was landed? @sergiz

[mcollina]

We are missing the 2nd point: error handler of the parent plugin should catch errors throw by the children plugins.

[sergejostir]

Could we close it since #3200 was landed? @sergiz

Not really. That PR addresses only the third section of this issue, I will open PRs that address other parts after the 1st of August.

[mcollina]

I'm preparing a PR to address encapsulation of error handlers and cascading errors between them.

[mcollina]

Closing as I think this was handled in #3261