Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoiding trying try/catch block when handling errors in Express? #200

Open
abdellani opened this issue May 3, 2021 · 1 comment
Open

Comments

@abdellani
Copy link

Hi

I was reading this blog ( https://github.com/rwieruch/blog_robinwieruch_content/blob/master/blog/node-express-error-handling/index.md ) about the handling errors in Express JS. Before sharing my note, I want to thank you for your efforts.

When you said: Fortunately we don't need to use a try/catch block but just use the promise's catch method instead.

router.post('/', async (req, res, next) => {
  const message = await req.context.models.Message.create({
    text: req.body.text,
    user: req.context.me.id,
  }).catch(next);

  return res.send(message);
});

I think that this is not correct. When the middleware finishes sending the error to the client, Express will try to run res.send(message). This will raise another exception because the code is trying to send another response to the client.
please correct me if I'm wrong.
Here's a copy of the error :

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

Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
    at new NodeError (node:internal/errors:329:5)
    at ServerResponse.setHeader (node:_http_outgoing:579:11)
    at ServerResponse.header (/home/.../node_modules/express/lib/response.js:771:10)
    at ServerResponse.send (/home/.../node_modules/express/lib/response.js:170:12)
    at ServerResponse.json (/home/.../node_modules/express/lib/response.js:267:15)
    at eval (webpack:///./src/controllers/users.js?:34:18)
    at processTicksAndRejections (node:internal/process/task_queues:94:5)
    at async createUser (webpack:///./src/controllers/users.js?:31:3) {
  code: 'ERR_HTTP_HEADERS_SENT'
}
[nodemon] app crashed - waiting for file changes before starting...

I think the correct way to do it, is wether by using try/catch block or by moving the res.send into then before the catch block, it should look like:

router.post('/', async (req, res, next) => {
  req.context.models.Message.create({
    text: req.body.text,
    user: req.context.me.id,
  }).then(()=>res.send(message))
  .catch(next);
});

The same thing for the following code.

...

import { BadRequestError } from '../utils/errors';

...

router.post('/', async (req, res, next) => {
  const message = await req.context.models.Message.create({
    text: req.body.text,
    user: req.context.me.id,
  }).catch((error) => next(new BadRequestError(error)));

  return res.send(message);
});
@abdellah711
Copy link

abdellah711 commented Mar 19, 2022

Hi!
You're totally right, the code of this article will crash the application whenever the database throws an error.
What you can do to solve this problem is to check first if the message is not undefined before sending the response
So the code would be like this :

router.post('/', async (req, res, next) => {
  const message = await req.context.models.Message.create({
    text: req.body.text,
    user: req.context.me.id,
  }).catch(next);

  message && res.send(message);
});

Note: the return keyword is optional, so you can remove it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants