gergelyke.github.io
gergelyke.github.io copied to clipboard
gergelyke
How to handle errors in Express
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);
It isn't necesssary to pass next, right?
Promise.resolve(fn(req, res))...
fn(req, res, next) returns a promise. What is the real need of wrapping it with Promise.resolve?
@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 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. :smile:
@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? :)
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
@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 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.
@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
@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.
Understood - I recommend reading Eran's great piece on the topic: https://medium.com/@eranhammer/catching-without-awaiting-b2cb7df45790
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);,
Thanks Chip! Yes, on purpose :) so with the first return, you can make sure that next won't get called
Does it make sense?
We can also patch router to add wrapper automatically (you may customize catchAsyncErrors for your boom needs)
https://gist.github.com/Paxa/da339f1f72732bb0ee42d342da9ff6f7
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.
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.
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.
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.
@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?
@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 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?
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.
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
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.
