BackendStack21 / restana

Restana is a lightweight and fast Node.js framework for building RESTful APIs.
MIT License
467 stars 27 forks source link

throw in Async route #81

Closed victor-yang-bot closed 4 years ago

victor-yang-bot commented 4 years ago

If i throw an error on async function crash the server.

  app.post('/auth/google', async (req: any, res: any) => {
    throw new Error('Invalid google login');
    return;
  });
jkyberneees commented 4 years ago

Hi @victor-a-rigacci , can you detail a bit more what crash the server means?

Can you please have a look at this example: https://github.com/jkyberneees/ana/blob/master/demos/error-handler.js Here I describe how error handling works.

sarriaroman commented 4 years ago

I'm having the same problem when environment is Production. On development is working like a charm but in production is throwing and crash ignoring the "errorHandler".

jkyberneees commented 4 years ago

Hi @sarriaroman, I would need to know more details on your service configuration. I can tell you nor 0http or restana consider environment variables on their implementation.

You can confirm with this example:

'use strict'
process.env.ENV = 'PRODUCTION'

const service = require('../index')({
  errorHandler (err, req, res) {
    console.log(`Unexpected error: ${err.message}`)
    res.send(err)
  }
})

service.use(async (req, res, next) => {
  try {
    await next()
  } catch (err) {
    return next(err)
  }
})

service.get('/throw', (req, res) => {
  throw new Error('Upps!')
})
service.use('/nested', router)

service.start()

It might be that you are registering connect like middlewares that are not returning next() value, and therefore not propagating the response promise. I recommend you to register the following middleware right before your routes:

service.use(async (req, res, next) => {
  try {
    await next()
  } catch (err) {
    return next(err)
  }
})

Regards

victor-yang-bot commented 4 years ago

Hi, work perfectly thanks!

deman4ik commented 4 years ago

Hello. I am also had the same issue. Would recommend to put this snippet into docs.

service.use(async (req, res, next) => {
  try {
    await next()
  } catch (err) {
    return next(err)
  }
})
jkyberneees commented 4 years ago

Hi @deman4ik thanks for your remark on this. Let me explain the reason for this issue, also I will add it to the readme.

Some middlewares don't do return next(), instead they just call next() to continue the middlewares execution chain. The second, is a bad practice as it silence any potential Promise rejection that happens in the downstream middlewares or handlers.

In restana (https://github.com/jkyberneees/ana/blob/master/index.js#L99) we enable async errors handling by default, however this mechanism fails when a subsequent middleware is registered containing the mentioned next() statement to finish their execution.

Thank you all for reporting.