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

Restify doesn't provide a way to catch Promise rejections in a middleware #1304

Closed
serge1peshcoff opened this Issue Apr 13, 2017 · 12 comments

Comments

Projects
None yet
8 participants
@serge1peshcoff

I want to be able to have a function that is called unhandled Promise rejections that happen inside a Restify middleware, to send the error to user. There is server.on('uncaughtException', ...) that catches all the errors, but if my middleware is async function or it returns a Promise, this handler won't catch it.

Example:

// server creation is omitted

server.on('uncaughtException', (req, res, route, err) => {
 console.log(err);
  res.send({ success: false, error: err.message });
});

server.get('/', async (res, req, next) => {
  throw new Error('Some error');
})

If I run this example, it won't call the error handler and will print this instead:

(node:23037) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: Some error

Restify 4.3.0, tested with Node.js 6.10.0 (compiled with Babel) and 7.9.0 (it supports async natively)

@Doeke

This comment has been minimized.

Show comment
Hide comment
@Doeke

Doeke Apr 24, 2017

I ran into this issue today, and created the following workaround:

function catchErrors(callback) {
  return async function errorHandler(req, res, next) {
    try {
      await callback(req, res, next)
    } catch (err) {
      if (!(err instanceof restify.errors.HttpError))
        err = new restify.errors.InternalServerError(err)
      next(err)
    }
  }
}

// wrap your async rest handlers

server.post('/posts', catchErrors(async (req, res, next) => {
  let data = req.body
  validate.newPost(data)
  await db.createObject('post', data)
  res.send(201) // CREATED
}))

// we can now modify or log our errors

server.on('InternalServer', function (req, res, err, next) {
  let cause = err.cause()
  if (cause && cause.name == 'ValidationError') {
    err.statusCode = 400
    err.body.code = 'ValidationError'
    err.body.message = cause.message
    err.body.details = cause.details
  } else {
    log.error(err)
    err.body = 'Something went wrong.'
  }
  next()
})

Doeke commented Apr 24, 2017

I ran into this issue today, and created the following workaround:

function catchErrors(callback) {
  return async function errorHandler(req, res, next) {
    try {
      await callback(req, res, next)
    } catch (err) {
      if (!(err instanceof restify.errors.HttpError))
        err = new restify.errors.InternalServerError(err)
      next(err)
    }
  }
}

// wrap your async rest handlers

server.post('/posts', catchErrors(async (req, res, next) => {
  let data = req.body
  validate.newPost(data)
  await db.createObject('post', data)
  res.send(201) // CREATED
}))

// we can now modify or log our errors

server.on('InternalServer', function (req, res, err, next) {
  let cause = err.cause()
  if (cause && cause.name == 'ValidationError') {
    err.statusCode = 400
    err.body.code = 'ValidationError'
    err.body.message = cause.message
    err.body.details = cause.details
  } else {
    log.error(err)
    err.body = 'Something went wrong.'
  }
  next()
})
@retrohacker

This comment has been minimized.

Show comment
Hide comment
@retrohacker

retrohacker Apr 27, 2017

Member

Hey @serge1peshcoff,

First, thank you for taking the time to open this feature request, and thank you @Doeke for taking the time to document that solution! ❤️

The restify codebase is designed to create semantically correct REST services optimized for stability and introspection. Promises are an exciting feature of ES6, but the tooling around them is still maturing. I'm not convinced that promises are at a point where we can say we wouldn't be sacrificing the debugability of the codebase for the convenience provided by supporting that language construct as a first class citizen.

I'm definitely open to discussing this more though, and can be convinced otherwise. I'd like to hear other maintainers thoughts as well, I wouldn't be surprised if my opinion is rather unpopular 😉. I would say that I'm strongly against introducing a compilation step such as babel for this project, it introduces a layer of indirection between the code that we maintain and what gets shipped as the final project. Even with sourcemaps, we would be sacrificing quite a bit in order to backport ES6 features to older versions of Node.

For now, if you would like to use some of the newer ES6 in your codebase, I would recommend wrapping them in a pattern that is callback friendly, @Doeke has an awesome example of this!

As an aside, I wouldn't compare promise rejections to uncaughtException. I would strongly discourage using the uncaughtException handler as a way to recover and continue operation of a server. Node core has some pretty strong opinions about saving a process using domains and catching a top level uncaughtException; I'm inclined to agree with them. For more on this, refer to issue #1308

I'm going to close this for now. If anyone has a strong opinion about me being wrong here, we can re-open and discuss 😄

Member

retrohacker commented Apr 27, 2017

Hey @serge1peshcoff,

First, thank you for taking the time to open this feature request, and thank you @Doeke for taking the time to document that solution! ❤️

The restify codebase is designed to create semantically correct REST services optimized for stability and introspection. Promises are an exciting feature of ES6, but the tooling around them is still maturing. I'm not convinced that promises are at a point where we can say we wouldn't be sacrificing the debugability of the codebase for the convenience provided by supporting that language construct as a first class citizen.

I'm definitely open to discussing this more though, and can be convinced otherwise. I'd like to hear other maintainers thoughts as well, I wouldn't be surprised if my opinion is rather unpopular 😉. I would say that I'm strongly against introducing a compilation step such as babel for this project, it introduces a layer of indirection between the code that we maintain and what gets shipped as the final project. Even with sourcemaps, we would be sacrificing quite a bit in order to backport ES6 features to older versions of Node.

For now, if you would like to use some of the newer ES6 in your codebase, I would recommend wrapping them in a pattern that is callback friendly, @Doeke has an awesome example of this!

As an aside, I wouldn't compare promise rejections to uncaughtException. I would strongly discourage using the uncaughtException handler as a way to recover and continue operation of a server. Node core has some pretty strong opinions about saving a process using domains and catching a top level uncaughtException; I'm inclined to agree with them. For more on this, refer to issue #1308

I'm going to close this for now. If anyone has a strong opinion about me being wrong here, we can re-open and discuss 😄

@retrohacker

This comment has been minimized.

Show comment
Hide comment
@retrohacker

retrohacker Apr 27, 2017

Member

Also, that snippet @Doeke provided could easily become a module as a nice compromise 😄 If you release it on npm we can point people there for a solution moving forward.

Member

retrohacker commented Apr 27, 2017

Also, that snippet @Doeke provided could easily become a module as a nice compromise 😄 If you release it on npm we can point people there for a solution moving forward.

@yunong

This comment has been minimized.

Show comment
Hide comment
@yunong

yunong Apr 27, 2017

Member

We don't recommend that you either swallow uncaught exceptions (something which promises do) or continue execution of your process after catching an exception. We feel strongly that this is an antipattern, and doesn't lend itself to running robust and reliable services in production. After all, that is one of the key value propositions of restify. This guide should give you some context around how to reliably handle errors in Node.

Therefore, I highly doubt restify will ever support any promises based interface in the future -- so if you're looking for Promises support -- restify is probably not your best bet.

With that said, we're certainly not telling folks how they should be using restify or any body of software. If you choose to use Promises or continue execution after an uncaught exception, you're free to, with all the consequences that they entail.

Member

yunong commented Apr 27, 2017

We don't recommend that you either swallow uncaught exceptions (something which promises do) or continue execution of your process after catching an exception. We feel strongly that this is an antipattern, and doesn't lend itself to running robust and reliable services in production. After all, that is one of the key value propositions of restify. This guide should give you some context around how to reliably handle errors in Node.

Therefore, I highly doubt restify will ever support any promises based interface in the future -- so if you're looking for Promises support -- restify is probably not your best bet.

With that said, we're certainly not telling folks how they should be using restify or any body of software. If you choose to use Promises or continue execution after an uncaught exception, you're free to, with all the consequences that they entail.

@Doeke

This comment has been minimized.

Show comment
Hide comment
@Doeke

Doeke Apr 28, 2017

Promises never swallow exceptions if you're using async / await properly. It's restify that's swallowing exceptions by not sending them to the uncaughtException handler.

Doeke commented Apr 28, 2017

Promises never swallow exceptions if you're using async / await properly. It's restify that's swallowing exceptions by not sending them to the uncaughtException handler.

@serge1peshcoff

This comment has been minimized.

Show comment
Hide comment
@serge1peshcoff

serge1peshcoff Apr 28, 2017

@yunong about swallowing uncaught exceptions: I didn't mean to just silently ignore them, I need this to send the information about the error to the user and then restart the process.
I understand that these situation shouldn't usually happen and all of the errors should be processed in the middleware itself, still I suppose that there could be situations where a developer didn't think of a situation where exception could happen. In that way, it would be useful no notify user about the error at least.

@yunong about swallowing uncaught exceptions: I didn't mean to just silently ignore them, I need this to send the information about the error to the user and then restart the process.
I understand that these situation shouldn't usually happen and all of the errors should be processed in the middleware itself, still I suppose that there could be situations where a developer didn't think of a situation where exception could happen. In that way, it would be useful no notify user about the error at least.

@DonutEspresso

This comment has been minimized.

Show comment
Hide comment
@DonutEspresso

DonutEspresso May 1, 2017

Member

Promises never swallow exceptions if you're using async / await properly. It's restify that's swallowing exceptions by not sending them to the uncaughtException handler.

I think it's more nuanced in this case. Since restify doesn't support the Promise construct natively, any promise returned by a handler isn't getting unwrapped via something like bluebird's done() method. If async/await does that natively, that's great!

Without unwrapping the promise, you're effectively stuck in the "domain" of the promise, even as you move on to the next function in your route handler (calling next). That means any errors that are thrown are effectively caught by the originating promise, rather than then restify domain.

Member

DonutEspresso commented May 1, 2017

Promises never swallow exceptions if you're using async / await properly. It's restify that's swallowing exceptions by not sending them to the uncaughtException handler.

I think it's more nuanced in this case. Since restify doesn't support the Promise construct natively, any promise returned by a handler isn't getting unwrapped via something like bluebird's done() method. If async/await does that natively, that's great!

Without unwrapping the promise, you're effectively stuck in the "domain" of the promise, even as you move on to the next function in your route handler (calling next). That means any errors that are thrown are effectively caught by the originating promise, rather than then restify domain.

@noinkling

This comment has been minimized.

Show comment
Hide comment
@noinkling

noinkling May 13, 2017

There's always the unhandledRejection event, from there you could throw an error and catch it with an uncaughtException handler if you wanted. Unless I'm not understanding something.

There's always the unhandledRejection event, from there you could throw an error and catch it with an uncaughtException handler if you wanted. Unless I'm not understanding something.

@serge1peshcoff

This comment has been minimized.

Show comment
Hide comment
@serge1peshcoff

serge1peshcoff May 13, 2017

@noinkling the point of all this thing was to get the error data and send it back to the client, and for this I need my req and res objects, I can't get them from unhandledRejection event

@noinkling the point of all this thing was to get the error data and send it back to the client, and for this I need my req and res objects, I can't get them from unhandledRejection event

@christophedebatz

This comment has been minimized.

Show comment
Hide comment
@christophedebatz

christophedebatz Dec 1, 2017

Hello, i'm confused about the restify team explainations. Promise are still maturing? Lol ?

christophedebatz commented Dec 1, 2017

Hello, i'm confused about the restify team explainations. Promise are still maturing? Lol ?

@retrohacker

This comment has been minimized.

Show comment
Hide comment
@retrohacker

retrohacker Dec 2, 2017

Member

Hey @christophedebatz,

Sorry if this came across the wrong way. There are different definitions of maturity based on your constraints. Long running processes have a different set of constraints than short lived processes, and managing a huge number of processes has a different set of constraints than managing only a few.

Restify is designed to be long lived and run at scale. We optimize for the ability to see into what the process is doing at every stage of the request lifecycle. This is handy when you need to get an understanding of what is going on across a fleet of 200K Node.js processes, both while they are running and after they crash.

Unfortunately, the current state of promises are a liability here 😞, though things are getting better thanks to many amazing people contributing an incredible amount of effort ❤️ . For an example of what I mean, check out the post-mortem working group in Node.js where the community is trying to solve the problem of capturing a promise's state in a core dump.

I'm not sure I agree with other maintainers that promises will never be a part of Restify. I actually am starting to feel like their inclusion will be inevitable as the Node.js ecosystem sees wider adoption of promises. I don't see a future where they wont find their way into our code base through our dependencies, at which point we will either rebuild the world or invest engineering effort in solving running promises at scale in long running processes. That being said, I don't believe today is that day.

If you, or others, feel passionate about their inclusion in Restify, I would recommend taking a pass at solving the core dump issue above. If a contributor/maintainer comes along and is motivated to tackle the problems preventing us from bringing promises into Restify, I would be happy to spend a few days reviewing the current state of promises and documenting what we are missing.

Member

retrohacker commented Dec 2, 2017

Hey @christophedebatz,

Sorry if this came across the wrong way. There are different definitions of maturity based on your constraints. Long running processes have a different set of constraints than short lived processes, and managing a huge number of processes has a different set of constraints than managing only a few.

Restify is designed to be long lived and run at scale. We optimize for the ability to see into what the process is doing at every stage of the request lifecycle. This is handy when you need to get an understanding of what is going on across a fleet of 200K Node.js processes, both while they are running and after they crash.

Unfortunately, the current state of promises are a liability here 😞, though things are getting better thanks to many amazing people contributing an incredible amount of effort ❤️ . For an example of what I mean, check out the post-mortem working group in Node.js where the community is trying to solve the problem of capturing a promise's state in a core dump.

I'm not sure I agree with other maintainers that promises will never be a part of Restify. I actually am starting to feel like their inclusion will be inevitable as the Node.js ecosystem sees wider adoption of promises. I don't see a future where they wont find their way into our code base through our dependencies, at which point we will either rebuild the world or invest engineering effort in solving running promises at scale in long running processes. That being said, I don't believe today is that day.

If you, or others, feel passionate about their inclusion in Restify, I would recommend taking a pass at solving the core dump issue above. If a contributor/maintainer comes along and is motivated to tackle the problems preventing us from bringing promises into Restify, I would be happy to spend a few days reviewing the current state of promises and documenting what we are missing.

@leodutra

This comment has been minimized.

Show comment
Hide comment
@leodutra

leodutra Dec 5, 2017

Similar discussion, of interest.
nodejs/promises#26

leodutra commented Dec 5, 2017

Similar discussion, of interest.
nodejs/promises#26

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