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

How to handle errors in Express #10

Open
gergelyke opened this Issue Nov 30, 2017 · 28 comments

Comments

9 participants
@gergelyke
Owner

gergelyke commented Nov 30, 2017

No description provided.

@gergelyke gergelyke added the comments label Nov 30, 2017

@nikola1970

This comment has been minimized.

Show comment
Hide comment
@nikola1970

nikola1970 Nov 30, 2017

Nice, thank you!

nikola1970 commented Nov 30, 2017

Nice, thank you!

@ruanmartinelli

This comment has been minimized.

Show comment
Hide comment
@ruanmartinelli

ruanmartinelli Nov 30, 2017

Good post Gergely!

Maybe it's worth adding that errors thrown in an async context won't be handled by the asyncMiddleware?

E.g. adding this code in a controller:

setTimeout(() => {
    throw new Error('Oops')
}, 2000);

ruanmartinelli commented Nov 30, 2017

Good post Gergely!

Maybe it's worth adding that errors thrown in an async context won't be handled by the asyncMiddleware?

E.g. adding this code in a controller:

setTimeout(() => {
    throw new Error('Oops')
}, 2000);
@robertmirro

This comment has been minimized.

Show comment
Hide comment
@robertmirro

robertmirro Nov 30, 2017

It isn't necesssary to pass next, right?

Promise.resolve(fn(req, res))...

robertmirro commented Nov 30, 2017

It isn't necesssary to pass next, right?

Promise.resolve(fn(req, res))...
@dolvik

This comment has been minimized.

Show comment
Hide comment
@dolvik

dolvik Dec 1, 2017

fn(req, res, next) returns a promise. What is the real need of wrapping it with Promise.resolve?

dolvik commented Dec 1, 2017

fn(req, res, next) returns a promise. What is the real need of wrapping it with Promise.resolve?

@dolvik

This comment has been minimized.

Show comment
Hide comment
@dolvik

dolvik Dec 1, 2017

@robertmirro Here in this example next is not used inside handler. But it can, e.g. when callback function is used:

app.get('/users/:id', asyncMiddleware(async (req, res, next) => {
  const userId = req.params.id
  if (!userId) {
    throw boom.badRequest('missing id')
  }
  const user = await Users.get(userId)

  doSomethingWithUser(user, (err) => {
    if (err) {
      return next(err);
    }
    res.json(user)
  });
}))

dolvik commented Dec 1, 2017

@robertmirro Here in this example next is not used inside handler. But it can, e.g. when callback function is used:

app.get('/users/:id', asyncMiddleware(async (req, res, next) => {
  const userId = req.params.id
  if (!userId) {
    throw boom.badRequest('missing id')
  }
  const user = await Users.get(userId)

  doSomethingWithUser(user, (err) => {
    if (err) {
      return next(err);
    }
    res.json(user)
  });
}))
@robertmirro

This comment has been minimized.

Show comment
Hide comment
@robertmirro

robertmirro Dec 1, 2017

@dolvik I thought the point of the final example was to avoid next in the callback and throw instead.

Also... I'm guessing that asyncMiddleware wraps the result of the callback in a promise for flexibility (callback isn't required to return a promise), but that's just a guess. 😄

robertmirro commented Dec 1, 2017

@dolvik I thought the point of the final example was to avoid next in the callback and throw instead.

Also... I'm guessing that asyncMiddleware wraps the result of the callback in a promise for flexibility (callback isn't required to return a promise), but that's just a guess. 😄

@tiendq

This comment has been minimized.

Show comment
Hide comment
@tiendq

tiendq Dec 5, 2017

@ruanmartinelli It's worth mentioning it. I threw an error in a async function and got error: Unhandled promise rejection Promise :)

Why the author doesn't reply? :)

tiendq commented Dec 5, 2017

@ruanmartinelli It's worth mentioning it. I threw an error in a async function and got error: Unhandled promise rejection Promise :)

Why the author doesn't reply? :)

@gergelyke

This comment has been minimized.

Show comment
Hide comment
@gergelyke

gergelyke Dec 5, 2017

Owner

Hi @ruanmartinelli @tiendq

I don't see that as a valid use case - if you want to add a delay in your controller, you should do something like this

app.get('/', asyncMw(async (req, res) => {
  function sleep (ms) {
     return new Promise ((resolve) => setTimeout(resolve, ms))
  }

  await sleep(2000)
}))

The whole point of this logic is not to add callbacks - this is how you can make it work with setTimeout. I hope it makes sense. Even if the code above throws, the asyncMw will catch it

Owner

gergelyke commented Dec 5, 2017

Hi @ruanmartinelli @tiendq

I don't see that as a valid use case - if you want to add a delay in your controller, you should do something like this

app.get('/', asyncMw(async (req, res) => {
  function sleep (ms) {
     return new Promise ((resolve) => setTimeout(resolve, ms))
  }

  await sleep(2000)
}))

The whole point of this logic is not to add callbacks - this is how you can make it work with setTimeout. I hope it makes sense. Even if the code above throws, the asyncMw will catch it

@tiendq

This comment has been minimized.

Show comment
Hide comment
@tiendq

tiendq Dec 5, 2017

@gergelyke Thank you, it makes sense now :)

tiendq commented Dec 5, 2017

@gergelyke Thank you, it makes sense now :)

@ruanmartinelli

This comment has been minimized.

Show comment
Hide comment
@ruanmartinelli

ruanmartinelli Dec 5, 2017

@gergelyke

The setTimeout was just an example!

Think of it as any async operation that doesn't need to be awaited at (maybe an endpoint that triggers some background processing?). If it throws, the asyncMiddleware won't catch it.

ruanmartinelli commented Dec 5, 2017

@gergelyke

The setTimeout was just an example!

Think of it as any async operation that doesn't need to be awaited at (maybe an endpoint that triggers some background processing?). If it throws, the asyncMiddleware won't catch it.

@tiendq

This comment has been minimized.

Show comment
Hide comment
@tiendq

tiendq Dec 5, 2017

@ruanmartinelli It was exactly my case too, but I think it was my code smell and I refactored it, if it's async I'll await, or I rewrite it as a normal (sync.) function.

tiendq commented Dec 5, 2017

@ruanmartinelli It was exactly my case too, but I think it was my code smell and I refactored it, if it's async I'll await, or I rewrite it as a normal (sync.) function.

@gergelyke

This comment has been minimized.

Show comment
Hide comment
@gergelyke

gergelyke Dec 5, 2017

Owner

@ruanmartinelli what kind of background processing you have in mind? if you want to do that, your endpoint should only dispatch a message to rabbitmq/nsq/etc, and let a separate process deal with that

Owner

gergelyke commented Dec 5, 2017

@ruanmartinelli what kind of background processing you have in mind? if you want to do that, your endpoint should only dispatch a message to rabbitmq/nsq/etc, and let a separate process deal with that

@ruanmartinelli

This comment has been minimized.

Show comment
Hide comment
@ruanmartinelli

ruanmartinelli Dec 5, 2017

@gergelyke Fair enough, but that is not my point.

What I am trying to say is that wrapping the code in asyncMiddleware-like functions can give a false sense of security, as if all errors thrown inside of the wrapper will be nicely handled by an express middleware. That is not true for the setTimeout example or any other errors thrown in an async context, which AFAIK crashes the app.

ruanmartinelli commented Dec 5, 2017

@gergelyke Fair enough, but that is not my point.

What I am trying to say is that wrapping the code in asyncMiddleware-like functions can give a false sense of security, as if all errors thrown inside of the wrapper will be nicely handled by an express middleware. That is not true for the setTimeout example or any other errors thrown in an async context, which AFAIK crashes the app.

@gergelyke

This comment has been minimized.

Show comment
Hide comment
@gergelyke

gergelyke Dec 5, 2017

Owner

Understood - I recommend reading Eran's great piece on the topic: https://medium.com/@eranhammer/catching-without-awaiting-b2cb7df45790

Owner

gergelyke commented Dec 5, 2017

Understood - I recommend reading Eran's great piece on the topic: https://medium.com/@eranhammer/catching-without-awaiting-b2cb7df45790

@chip

This comment has been minimized.

Show comment
Hide comment
@chip

chip Jan 25, 2018

Nice article! Thanks for posting. Why did you call return on return next(boom.badImplementation(err));, but 2 lines below you didn't next(err);,

chip commented Jan 25, 2018

Nice article! Thanks for posting. Why did you call return on return next(boom.badImplementation(err));, but 2 lines below you didn't next(err);,

@gergelyke

This comment has been minimized.

Show comment
Hide comment
@gergelyke

gergelyke Jan 25, 2018

Owner

Thanks Chip! Yes, on purpose :) so with the first return, you can make sure that next won't get called

Does it make sense?

Owner

gergelyke commented Jan 25, 2018

Thanks Chip! Yes, on purpose :) so with the first return, you can make sure that next won't get called

Does it make sense?

@chip

This comment has been minimized.

Show comment
Hide comment
@chip

chip Jan 25, 2018

@gergelyke - Of course! I missed the obvious. Thanks for clarifying. 👍

chip commented Jan 25, 2018

@gergelyke - Of course! I missed the obvious. Thanks for clarifying. 👍

@Paxa

This comment has been minimized.

Show comment
Hide comment
@Paxa

Paxa Feb 11, 2018

We can also patch router to add wrapper automatically (you may customize catchAsyncErrors for your boom needs)
https://gist.github.com/Paxa/da339f1f72732bb0ee42d342da9ff6f7

Paxa commented Feb 11, 2018

We can also patch router to add wrapper automatically (you may customize catchAsyncErrors for your boom needs)
https://gist.github.com/Paxa/da339f1f72732bb0ee42d342da9ff6f7

@ajxs

This comment has been minimized.

Show comment
Hide comment
@ajxs

ajxs Apr 28, 2018

What is the point of asyncMiddleware exactly? It's absolutely, completely and totally redundant.
You can just use an asynchronous function as your middleware, so what's the point?
As for your error handling methodology, a much better model would be to simply wrap your async functionality inside a try... catch block, and just use catch(e) { return next(e) } or similar to forward the exception to your error handling middleware.
You can accomplish pretty much all of this with native functionality and existing software development patterns.

ajxs commented Apr 28, 2018

What is the point of asyncMiddleware exactly? It's absolutely, completely and totally redundant.
You can just use an asynchronous function as your middleware, so what's the point?
As for your error handling methodology, a much better model would be to simply wrap your async functionality inside a try... catch block, and just use catch(e) { return next(e) } or similar to forward the exception to your error handling middleware.
You can accomplish pretty much all of this with native functionality and existing software development patterns.

@nikola1970

This comment has been minimized.

Show comment
Hide comment
@nikola1970

nikola1970 Apr 28, 2018

The point is exactly that - do NOT write try/catch block in every handler. Imaging you having many routes and you want to handle them, for every you write try/catch...

Async middleware prevents code repeat (try/catch blocks) and you have final error handling middleware at the end of the all route handlers which picka up an error.

nikola1970 commented Apr 28, 2018

The point is exactly that - do NOT write try/catch block in every handler. Imaging you having many routes and you want to handle them, for every you write try/catch...

Async middleware prevents code repeat (try/catch blocks) and you have final error handling middleware at the end of the all route handlers which picka up an error.

@ajxs

This comment has been minimized.

Show comment
Hide comment
@ajxs

ajxs Apr 28, 2018

I don't see the issue with this pattern. It's worked very well for C++/Java/etc for decades.
You can't possibly call try/catch code repetition can you? It's part of the native Javascript syntax for a reason. You've also removed all of the flexibility that try/catch offers. What if I want to use catch together with finally to recover from an error and return the application to a usable state? What if I want different parts of my middleware to catch different kinds of exceptions and handle them differently? Is every exception fatal in your model?
The pattern you're using is extremely inflexible, and creates unnecessary complexity. I have no idea why you'd consider try/catch boilerplate, considering it's an extremely well established pattern/paradigm...
One extremely common anti-pattern I see from Javascript developers is the misuse of Promise.reject(...). When using async/await promise rejection is analogous to throwing an exception, which should not be used for business logic. A good rule-of-thumb is to throw or reject only when assumptions made by your business logic have not been met, eg. null parameters and such.

ajxs commented Apr 28, 2018

I don't see the issue with this pattern. It's worked very well for C++/Java/etc for decades.
You can't possibly call try/catch code repetition can you? It's part of the native Javascript syntax for a reason. You've also removed all of the flexibility that try/catch offers. What if I want to use catch together with finally to recover from an error and return the application to a usable state? What if I want different parts of my middleware to catch different kinds of exceptions and handle them differently? Is every exception fatal in your model?
The pattern you're using is extremely inflexible, and creates unnecessary complexity. I have no idea why you'd consider try/catch boilerplate, considering it's an extremely well established pattern/paradigm...
One extremely common anti-pattern I see from Javascript developers is the misuse of Promise.reject(...). When using async/await promise rejection is analogous to throwing an exception, which should not be used for business logic. A good rule-of-thumb is to throw or reject only when assumptions made by your business logic have not been met, eg. null parameters and such.

@nikola1970

This comment has been minimized.

Show comment
Hide comment
@nikola1970

nikola1970 Apr 28, 2018

I have to agree with you that the pattern is inflexible though. I tried it and found that I do not have fine control over my responses and I ditched it completely and went back to Promises.

nikola1970 commented Apr 28, 2018

I have to agree with you that the pattern is inflexible though. I tried it and found that I do not have fine control over my responses and I ditched it completely and went back to Promises.

@gergelyke

This comment has been minimized.

Show comment
Hide comment
@gergelyke

gergelyke Apr 28, 2018

Owner

@ajxs as Express does not support Promise-based route handlers, you have to write a wrapper to send all errors to the next function. Alternatively, you can do this in all route handlers too, but in that case all your async route handlers would start with a try-catch, and call next in the catch. Either works.

Did I answer your question?

Owner

gergelyke commented Apr 28, 2018

@ajxs as Express does not support Promise-based route handlers, you have to write a wrapper to send all errors to the next function. Alternatively, you can do this in all route handlers too, but in that case all your async route handlers would start with a try-catch, and call next in the catch. Either works.

Did I answer your question?

@ajxs

This comment has been minimized.

Show comment
Hide comment
@ajxs

ajxs Apr 28, 2018

@gergelyke what do you mean Express does not support Promise-based route handlers? It absolutely does in the form of async functions. Obviously you can't use the Promise API resolve and reject to affect control flow in an express app, but you can use async functions directly as middleware and avoid creating a wrapper function that returns middleware.

all your async route handlers would start with a try-catch, and call next in the catch

Yes, that's exactly what I would advise. Not only does this accurately 'describe' the code's intent, to catch exceptions and forward them to error-handling middleware, but it allows you to declare individual try/catch blocks to handle exceptions in various parts of your code differently.

There's no need to try and reinvent the wheel when it comes to exception handling, the existing patterns were pretty tried, tested and battle hardened.

ajxs commented Apr 28, 2018

@gergelyke what do you mean Express does not support Promise-based route handlers? It absolutely does in the form of async functions. Obviously you can't use the Promise API resolve and reject to affect control flow in an express app, but you can use async functions directly as middleware and avoid creating a wrapper function that returns middleware.

all your async route handlers would start with a try-catch, and call next in the catch

Yes, that's exactly what I would advise. Not only does this accurately 'describe' the code's intent, to catch exceptions and forward them to error-handling middleware, but it allows you to declare individual try/catch blocks to handle exceptions in various parts of your code differently.

There's no need to try and reinvent the wheel when it comes to exception handling, the existing patterns were pretty tried, tested and battle hardened.

@gergelyke

This comment has been minimized.

Show comment
Hide comment
@gergelyke

gergelyke Apr 29, 2018

Owner

@ajxs as far as I know, if you do the following:

app.use(async function (req, res) {
  // just to imitate an async db connection
  setTimeout(() => {
    throw new Error('')  
  })
})

then the request will hang, as nothing passes the Error instance to the next method - am I missing something?

Owner

gergelyke commented Apr 29, 2018

@ajxs as far as I know, if you do the following:

app.use(async function (req, res) {
  // just to imitate an async db connection
  setTimeout(() => {
    throw new Error('')  
  })
})

then the request will hang, as nothing passes the Error instance to the next method - am I missing something?

@ajxs

This comment has been minimized.

Show comment
Hide comment
@ajxs

ajxs Apr 29, 2018

No, you're not. You'll still need to call next, as Express routers don't catch exceptions and forward them automatically. But that's moving the goalposts a little.

const ridiculousPromise = (testString) => new Promise((resolve, reject) => {
	setTimeout(function printString() {
		resolve(testString);
	}, 2000);
}).catch(e => {
	reject(e);
});


async function completelyAsync(req, res, next) {
	try {
		const ridiculousValue = await ridiculousPromise("TESTING");
		res.send(ridiculousValue);
	} catch(e) {
		next(e);
	}
}


app.get("/", completelyAsync);

This example works pretty much exactly as you'd expect. I don't see anything wrong with just calling next(...) in the route handler explicitly. In my actual node.js work I frequently want to be able to recover from certain errors without forwarding them onto the error handler, so I need the flexibility this pattern affords. Plus I feel like wrapper-functions are anti-pattern in 99% of cases anyway.

ajxs commented Apr 29, 2018

No, you're not. You'll still need to call next, as Express routers don't catch exceptions and forward them automatically. But that's moving the goalposts a little.

const ridiculousPromise = (testString) => new Promise((resolve, reject) => {
	setTimeout(function printString() {
		resolve(testString);
	}, 2000);
}).catch(e => {
	reject(e);
});


async function completelyAsync(req, res, next) {
	try {
		const ridiculousValue = await ridiculousPromise("TESTING");
		res.send(ridiculousValue);
	} catch(e) {
		next(e);
	}
}


app.get("/", completelyAsync);

This example works pretty much exactly as you'd expect. I don't see anything wrong with just calling next(...) in the route handler explicitly. In my actual node.js work I frequently want to be able to recover from certain errors without forwarding them onto the error handler, so I need the flexibility this pattern affords. Plus I feel like wrapper-functions are anti-pattern in 99% of cases anyway.

@gergelyke

This comment has been minimized.

Show comment
Hide comment
@gergelyke

gergelyke Apr 29, 2018

Owner

Yeah, it is about trade-offs - yours is a perfect solution too imho.

Also, even with the wrapped version, you can do try-catch and local error handling, if you'd like to recover from any error cases

Owner

gergelyke commented Apr 29, 2018

Yeah, it is about trade-offs - yours is a perfect solution too imho.

Also, even with the wrapped version, you can do try-catch and local error handling, if you'd like to recover from any error cases

@tiendq

This comment has been minimized.

Show comment
Hide comment
@tiendq

tiendq Apr 29, 2018

Also, even with the wrapped version, you can do try-catch and local error handling, if you'd like to recover from any error cases

This is what I do with my apps, most of the time I lets it goes to generic async middleware function and only use try/catch locally where needed.

tiendq commented Apr 29, 2018

Also, even with the wrapped version, you can do try-catch and local error handling, if you'd like to recover from any error cases

This is what I do with my apps, most of the time I lets it goes to generic async middleware function and only use try/catch locally where needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment