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

Catches errors from Promises / Async functions #3053

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@leebyron

leebyron commented Aug 13, 2016

This PR adds support for catching errors from rejected promises (perhaps in the form of an async function) that matches the existing try/catch behavior for synchronous returning functions.

Promises have now been part of the JavaScript specification for over a year and are popularly used for modeling asynchronous behavior (such as rendering a webpage). They're even mentioned in an express article about best practices.

However one of the pitfalls of using Promises is being aware of error behavior and not forgetting to call .catch(). This is a prominent note in that same article. However a common behavior is to "chain" Promises and allow for the top-most Promise to handle the error. For libraries where error-handling is critical, like webservers or test harnesses, being Promise-aware and allowing functions to return Promises can help isolate error behavior from the library user. Mocha is a great example of this done well. Users of Express may inadvertently expect this behavior, return a Promise from their routing functions, and then be surprised when errors are swallowed during debugging.

Support for Promises will become more critical as Async functions enters the language, giving first-class language syntax for creating and operating on promises. This proposal is expected to be ratified soon, is already implemented in Chakra-node, is implemented behind a flag in Chrome, is expected to arrive in node v7, and is already used by many via transpilers like Babel. This makes proper error handling for Promises imminent as this use case is about to become far more common. It would be great for Express to get ahead of this sooner rather than later.

Example of code where Express error handling will work just fine:

app.get('/', (req, res) => {
  throw new Error('Oh crap!')
})

And a minor syntax modification leads to swallowed errors:

app.get('/', async (req, res) => {
  throw new Error('Oh crap!')
})

This PR approaches a solution to the problem as minimally-invasive as possible. It does not presume anything about the host environment, and it detects and supports the Promise A subset of the Promise specification, which many older Promise libraries only support.

Catches errors from Promises / Async functions
This PR adds support for catching errors from rejected promises (perhaps in the
form of an async function) that matches the existing try/catch behavior for
synchronous returning functions.

[Promises](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise)
have now been part of the JavaScript specification for over a year and are
popularly used for modeling asynchronous behavior (such as rendering a webpage).
They're even mentioned in an express [article about best practices](https://expressjs.com/en/advanced/best-practice-performance.html#use-promises).

However one of the pitfalls of using Promises is being aware of error
behavior and not forgetting to call `.catch()`. This is a prominent note in that
same article. However a common behavior is to "chain" Promises and allow for the
top-most Promise to handle the error. For libraries where error-handling is
critical, like webservers or test harnesses, being Promise-aware and allowing
functions to return Promises can help isolate error behavior from the library
user. Mocha is a [great example of this done well](http://paulsalaets.com/testing-with-promises-in-mocha). Users of Express may inadvertently expect this
behavior, return a Promise from their routing functions, and then be surprised
when errors are swallowed during debugging.

Support for Promises will become more critical as [Async functions](https://tc39.github.io/ecmascript-asyncawait/) enters the language, giving first-class
language syntax for creating and operating on promises. This proposal is
expected to be ratified soon, is already implemented in Chakra-node, is
implemented behind a flag in Chrome, is expected to arrive in node v7, and is
already used by many via transpilers like Babel. This makes proper error
handling for Promises imminent as this use case is about to become far
more common.

Example of code where Express error handling will work just fine:

```js
app.get('/', (req, res) => {
  throw new Error('Oh crap!')
})
```

And a minor syntax modification leads to swallowed errors:

```js
app.get('/', async (req, res) => {
  throw new Error('Oh crap!')
})
```

This PR approaches a solution to the problem as minimally-invasive as possible.
It does not presume anything about the host environment, and it detects and
supports the [Promise A](http://wiki.commonjs.org/wiki/Promises/A) subset of the
Promise specification, which many older Promise libraries only support.
@leebyron

This comment has been minimized.

leebyron commented Aug 13, 2016

This is a limited subset of the topics of discussion raised in #2259, only error handling.

Hopefully great won't be the enemy of good in making this incremental improvement.

@dougwilson

This comment has been minimized.

Member

dougwilson commented Aug 14, 2016

Hi @leebyron, thank you so much for the pull request! Unfortunately we did try to add it to 4.x, but that is not possible, because there is current ecosystem code that is returning promises from middleware & handlers and does not expect Express.js to catch any errors (and when we do this, it typically ends up calling next(err) twice and Express.js cutting off the socket, as one main example). We also have PR #2809 open regarding this.

Rather than opening a duplicate PR, would you be willing to work with the author of #2809 on whatever needs to be done there (if anything)? Or at least, can you explain how this PR differs from this one so we can decide which PR to keep open?

Can you also target your PR against the 5.0 branch?

@leebyron

This comment has been minimized.

leebyron commented Aug 14, 2016

I'll target 5.0, sure.

This appears similar to #2809, however this PR's promise detection should be more generally applicable (not all Promises follow the A+ standard and implement a catch method). This PR also adds tests.

So I understand correctly, does rebasing on 5.0 alleviate concerns about breaking any existing uses? Or if it does not, then I'd love to know more about the specifics of those cases to protect against.

@dougwilson

This comment has been minimized.

Member

dougwilson commented Aug 14, 2016

AFAIK, there is no way to make it compatible, which is the purpose of targeting 5.0, since it's a major version, it is not expected to be perfectly compatible and it's the chance we have to make changes that will break existing servers :)

@leebyron

This comment has been minimized.

leebyron commented Aug 14, 2016

Great, just double checking.

Also, just to make sure I understand the backwards-compatible issue, does this look right?

// 1) backwards compatible, error was caught in user-code, won't be caught by
// Express
app.get('/', (req, res, next) => {
  return promiseSomething()
    .catch(next)
})

// 2) behavior changes. Previously errors were swallowed (intentional? unintentional?)
// and this changes behavior to forward errors from a rejected promise to the
// next Express error handler.
app.get('/', (req, res, next) => {
  return promiseSomething()
})

// 3) behavior changes. Rejected promise was handled both in user-code and forwarded
// to be handled by Express.
//
// Is this theoretical, or are there examples in the ecosystem of code like this?
app.get('/', (req, res, next) => {
  return promiseSomething().
    catch(err => {
      next(err)
      throw err
    })
})

Case 1) seems like what I would expect to find throughout the ecosystem, but that will have no changes.

Case 2) would be a behavior change, but it could also be argued to be a bug fix (unless the bug is considered a feature at this point).

Case 3) seems like the only really clear back-compat issue, but I'm struggling to think of how code like that ends up being written and am curious if it's a theoretical concern or if you've actually come across examples like this in your experience.

@dougwilson

This comment has been minimized.

Member

dougwilson commented Aug 14, 2016

Hi @leebyron, if your changes is similar to #2809, then it does not work in 4.x. I tried it with real code quite a long time ago. I'll try to dig up all those old findings, but at this point, it feels like you rather argue rather than simply help move this forward. At this point, there is no way your change is going to land on 4.x; please target 5.0 and let's just move on with getting this moving forward :)

@leebyron

This comment has been minimized.

leebyron commented Aug 14, 2016

Sorry, I didn't mean to argue, just gain understanding. I'll have a 5.0 targeted version up pronto

@dougwilson

This comment has been minimized.

Member

dougwilson commented Aug 14, 2016

I only close this, since this is a PR and you cannot retarget a PR (you have to make a new PR against a different branch). If this was an issue, of course I would leave it open :) The more conversation that happens in here will just end up lost once the PR is re-targeted it all, so I just didn't want to end up with a lot of conversation in a PR that would have ended up closed anyway. I hope that makes sense :)

@leebyron

This comment has been minimized.

leebyron commented Aug 14, 2016

👍

@dougwilson dougwilson removed the invalid label Aug 14, 2016

@dougwilson

This comment has been minimized.

Member

dougwilson commented Aug 14, 2016

And for reference as well, the only real thing at this point keeping Express 5.0 from being a thing is just thinking on this whole promise thing, so targeting it at 5.0 doesn't really make take longer to get released, if that is the fear (I've hear it a few times), as it's the main thing we're waiting to get done.

The main part that hasn't actually been done is that with the handing of promises, this fundamentally changes the way people will write handlers/middleware, and there are many libraries out there that use these patterns, so we just need to actually get defined the way in which people will create and work with promise-based handlers.

The absolute biggest issue, if you were wondering, is that a lot of people and libraries wrap other middleware/handlers, but do not pass through the return value of what they are wrapping. This is an issue in the ecosystem such that it pretty much makes it not useful to handle return values ourselves, since users will experience this as Express bugs. Putting it in Express 5.0 gives us a good messaging around this, giving people a way to say if their module is "Express 5 compatible" and makes it easier for users to understand if a particular module is likely to break this or not, if that makes sense.

There are other issues as well, but this issue is certainly a support nightmare :)

@leebyron

This comment has been minimized.

leebyron commented Aug 14, 2016

Aha, thanks for that info. Wrapping middleware and not passing values through would definitely lead to confusion.

Let me know if I can be of any assistance beyond this specific error handling functionality for Promise behavior in express@5

@leebyron leebyron deleted the leebyron:async-throw branch Aug 14, 2016

@defunctzombie

This comment has been minimized.

Contributor

defunctzombie commented Sep 3, 2016

@dougwilson could this be added to v4 under a feature flag (or is v5 ready to go soon?)

@dougwilson

This comment has been minimized.

Member

dougwilson commented Sep 3, 2016

Blake implemented a pretty neat version that even supports upstream. Either that or this will be in the next 5.0 release.

@defunctzombie

This comment has been minimized.

Contributor

defunctzombie commented Sep 3, 2016

@dougwilson do you have a link to that version? Maybe released as a module that will monkey patch?

/cc @blakeembrey

@dougwilson

This comment has been minimized.

Member

dougwilson commented Sep 3, 2016

I'm on my phone, so not easy to get a link real quick. In express 5.0, all the routing stuff was split into the "router" npm module. The PR is in that module. This means that you can easily use the promises stuff with express of any version.

@defunctzombie

This comment has been minimized.

Contributor

defunctzombie commented Sep 3, 2016

Awesome! Thanks.

On Saturday, September 3, 2016, Douglas Wilson notifications@github.com
wrote:

I'm on my phone, so not easy to get a link real quick. In express 5.0, all
the routing stuff was split into the "router" npm module. The PR is in that
module. This means that you can easily use the promises stuff with express
of any version.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#3053 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFLOLqF9HNYTr96ySeeYaROiOxmG2y7ks5qmgBKgaJpZM4JjyQo
.

@blakeembrey

This comment has been minimized.

Member

blakeembrey commented Sep 4, 2016

@dougwilson @defunctzombie I ended up coming to the conclusion that the official router should stay the same - without upstream support - as it just affects too much with limited value as upstream can be moved to a separate router instead of core. It should definitely catch promises though, the PR by me is at pillarjs/router#32 while @leebyron's PR should probably be used at pillarjs/router#47. There's also a lot of other PRs in there 😄

@defunctzombie

This comment has been minimized.

Contributor

defunctzombie commented Sep 4, 2016

@blakeembrey @dougwilson Sorry if this was stated elsewhere, but could you clarify what you mean by "upstream" support. I have seen this phrase used a few times now and do not understand what it refers to.

@blakeembrey

This comment has been minimized.

Member

blakeembrey commented Sep 5, 2016

@defunctzombie It's all good. It refers to a more Koa-like interface, where you can call next() which would return a promise that resolves when "downstream" finishes so you can handle things that come after it. In that fashion, middleware becomes bidirectional as you can go down the stack, but it'll also unwind back up. Here's an example using middleware like that:

function (req, res, next) {
  var start = Date.now()

  return next().then(function () {
    var end = Date.now()

    console.log(end - start)
  })
}

It also means error handling, instead of propagating forwards, turns into a catch as it unwinds. I've been using the pattern for HTTP requests (instead of the server) for https://github.com/blakeembrey/popsicle and have found it very useful when it comes to middleware. That's not to say it'll have the same benefits if it were implemented in Express, it's just a different approach.

However, it's also a much different pattern which I found broke too much existing code and it's possible to make a custom router that can mount the existing style anyway (since next() is not returned using callbacks in existing middleware). A call to next() means go next middleware and upstream isn't currently handled, so you can wrap these routers:

function (req, res, next) {
  const p = new Promise((resolve, reject) {
    old(req, res, function (err) {
      return err ? reject(err) : resolve()
    })
  })

  return p.then(next)
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment