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

throw in Async route #81

Closed
victor-yang-bot opened this issue Apr 17, 2020 · 6 comments
Closed

throw in Async route #81

victor-yang-bot opened this issue Apr 17, 2020 · 6 comments

Comments

@victor-yang-bot
Copy link

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
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator

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
Copy link
Author

Hi, work perfectly thanks!

@deman4ik
Copy link

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
Copy link
Collaborator

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.

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

4 participants