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

promises #2259

Closed
jonathanong opened this Issue Jul 24, 2014 · 63 comments

Comments

Projects
None yet
@jonathanong
Member

jonathanong commented Jul 24, 2014

now that promises are going mainstream, i'm trying to think of how to make express more async friendly. an idea is to use promises.

  • next() now returns a promise
  • if middleware returns a promise, that promise is resolved and propagated up next()s
app.use(function (req, res, next) {
  // a promise must be returned, 
  // otherwise the function will be assumed to be synchronous
  return User.get(req.session.userid).then(function (user) {
    req.user = user
  })
  .then(next) // execute all downstream middleware
  .then(function () {
    // send a response after all downstream middleware have executed
    // this is equivalent to koa's "upstream"
    res.send(user)
  })
})

Error handlers are now more koa-like:

app.use(function (req, res, next) {
  return next().catch(function (err) {
    // a custom error handler
    if (res.headerSent) return
    res.statusCode = err.status || 500
    res.send(err.message) 
  })
})

app.use(function (req, res) {
  throw new Error('hahahah')
})

Pros:

  • it should be backwards compatible since you don't have to resolve the promise returned from next()
  • much easier error handling including throwing
  • solves issues shown in #2255
  • no more fn.length checking _
  • could probably easily upgrade to es7 async functions

Cons:

  • promises
  • upgrading middleware and supporting both signatures might be a pain in the ass
  • probably a lot slower

@jonathanong jonathanong added the Ideas label Jul 24, 2014

@dougwilson

This comment has been minimized.

Member

dougwilson commented Jul 24, 2014

it should be backwards compatible since you don't have to resolve the promise returned from next()

that statement conflicts with "a promise must be returned, otherwise the function will be assumed to be synchronous" i would think, right?

much easier error handling including throwing

yep

solves issues shown in #2255

no, it does not solve that issue at all. that issue is to be able to re-write content heading out to res.write and friends; this is just the flow control for middleware processing.

no more fn.length checking _

amen

probably a lot slower

it is in that it does really increase the individual latency on requests.

i.m.o we can't really have it until nodejs/node-v0.x-archive#7714 is fixed (or at least, promised to be fixed).

@dougwilson

This comment has been minimized.

Member

dougwilson commented Jul 24, 2014

overall, though, I think promises are a good direction, but we certainly have a lot of issues to iron out (especially the Node.js core issue).

@jonathanong

This comment has been minimized.

Member

jonathanong commented Jul 24, 2014

that statement conflicts with "a promise must be returned, otherwise the function will be assumed to be synchronous" i would think, right?

well if you call next() without returning it through some promise pipeline, you basically just mess up "upstream". it'll just act as a callback. pretty sure this is an edge case we'd have to sort out if we actually implement it

@dougwilson

This comment has been minimized.

Member

dougwilson commented Jul 24, 2014

Well, I mean, it sounds like you're saying this would no longer work, right?

app.use(function (req, res, next) {
  // fetching from db
  setTimeout(function () {
    req.prop = 'something'
    next()
  }, 1000)
})
@jonathanong

This comment has been minimized.

Member

jonathanong commented Jul 24, 2014

upstream won't work (next().catch()), but downstream (i.e. how it works now) should still work

@dougwilson

This comment has been minimized.

Member

dougwilson commented Jul 24, 2014

upstream won't work (next().catch())

so you're saying next(err) in the above wouldn't be able to make it through the new-style error handlers? that sounds like a big problem to me. if it has to be like that, then it would sound like we could only work with Promises.

@vkurchatkin

This comment has been minimized.

vkurchatkin commented Jul 24, 2014

i.m.o we can't really have it until nodejs/node-v0.x-archive#7714 is fixed (or at least, promised to be fixed).

if promise support doesn't require using native promises, then this is ok. But it should probably be well documented that native promises should not be used at the time (e. g. all examples should use bluebird or something else)

@dougwilson

This comment has been minimized.

Member

dougwilson commented Jul 24, 2014

But it should probably be well documented that native promises should not be used at the time

Yea, no. People don't read it. Then they either just throw away express when their requests just hang or come and post many an issue on our issue tracker. Besides, people will give us promises from the ORM. How would they know if their ORM is using the correct type of promises or not?

@vkurchatkin

This comment has been minimized.

vkurchatkin commented Jul 24, 2014

Yea, no. People don't read it

well, in this case they wouldn't know about this feature

Besides, people will give us promises from the ORM.

this will broken anyways, doesn't matter if express supports promises or not

@dougwilson

This comment has been minimized.

Member

dougwilson commented Jul 24, 2014

this will broken anyways, doesn't matter if express supports promises or not

people will blame it on express. i'm not sure why there is even an argument here. node.js core fixes their workings with native promises, then express supports promises. we have a clear path forward here.

@vkurchatkin

This comment has been minimized.

vkurchatkin commented Jul 24, 2014

i mean, it's broken already:

orm.getThing().then(function (thing) {
  res.send(thing);
})

also callback-based libs can forget to call a callback, which leads to exactly same result. Nothing new

@dougwilson

This comment has been minimized.

Member

dougwilson commented Jul 24, 2014

And? Just fix that Joynet issue first. That's all I'm saying. Perhaps channel your energy into helping Joyent fix their promises :)

@vkurchatkin

This comment has been minimized.

vkurchatkin commented Jul 24, 2014

I've started working on the fix, in fact. What I'm saying, is that issue is terrible, but totally unrelated.

@dougwilson

This comment has been minimized.

Member

dougwilson commented Jul 24, 2014

but totally unrelated.

it is not unrelated

@vkurchatkin

This comment has been minimized.

vkurchatkin commented Jul 24, 2014

again, what you are saying WILL be broken, is broken ALREADY. support for promises != support for native promises

@dougwilson

This comment has been minimized.

Member

dougwilson commented Jul 24, 2014

support for promises != support for native promises

no, support for promises means people will actually start using them more actively. people who are using them now have already figured this stuff out if they are the kind of person who uses 0.11. the other casual users are using 0.10 which doesn't even have native promises.

express here is about being a very stable platform. this is also something that would not be included until a 5.0 release, which has no timeline currently, so there is no reason to rush anything here. everything @jonathanong has described can also be implemented in a user-land router and thus you can use the whole describe promise thing with a third-party promise route on express 4 or 3 today. we are talking about adding this to core express and as such it needs to be a good experience for our users. that's pretty much the end of the story here.

@defunctzombie

This comment has been minimized.

Contributor

defunctzombie commented Jul 24, 2014

-1000

@bajtos

This comment has been minimized.

bajtos commented Jul 24, 2014

While I don't like the idea of keeping promises out of express because the native implementation of Node.js core is broken, the idea of creating a new project for the promise-enabled router sounds great to me. It will allow us to experiment with different APIs, get feedback from users that are brave enough to test the experimental promise support, all that without locking down the core express API. Once we have experimented enough, gathered enough knowledge and the Node.js core has a working implementation of promises, then we can move the promise-enabled router to express core.

@dougwilson

This comment has been minimized.

Member

dougwilson commented Jul 24, 2014

Yes, no one said express will never have a promise-based router, but just that there is nothing definitive yet except. The current state of Node.js w.r.t Promises would keep us from committing them to core, but it's something fixable and it's a bit irritating that the issue is so downplayed and ignored, especially since it can cause the entire Route to just seize up in certain situations.

We are, in fact, working on moving the entire express router out into it's own module (and providing it as the default router). That of course doesn't really change the fact that even since at least express 3, you can use your own router as much as you want, even a Promise-based one :)

@jonathanong

This comment has been minimized.

Member

jonathanong commented Jul 24, 2014

i built kernel last night: https://github.com/jonathanong/promise-kernel/blob/master/index.js it's super slow.

@vkurchatkin

This comment has been minimized.

vkurchatkin commented Jul 24, 2014

@jonathanong on my machine:

  1 middleware
  6750.73

  5 middleware
  5148.63

  10 middleware
  3906.36

  15 middleware
  3619.97

  20 middleware
  3275.99

  30 middleware
  2732.74

  50 middleware
  1853.73

  100 middleware
  1140.98

with bluebird:

  1 middleware
  7195.06

  5 middleware
  7412.70

  10 middleware
  7747.73

  15 middleware
  6403.76

  20 middleware
  6772.03

  30 middleware
  5775.99

  50 middleware
  5571.38

  100 middleware
  5330.84

koa:

  1 middleware
  3652.71

  5 middleware
  3840.43

  10 middleware
  3960.29

  15 middleware
  3904.79

  20 middleware
  3669.25

  30 middleware
  3076.48

  50 middleware
  3691.57

  100 middleware
  3218.23
@dougwilson

This comment has been minimized.

Member

dougwilson commented Jul 24, 2014

@vkurchatkin for comparison, post the benchmarks from the https://github.com/visionmedia/express/tree/slim-benchmark ; benchmarks can only be compared to each other when run on the same machine, so we'd like to see how the express 4.6.1 router compares.

@vkurchatkin

This comment has been minimized.

vkurchatkin commented Jul 24, 2014

@dougwilson

  1 middleware
  5737.57

  5 middleware
  6047.46

  10 middleware
  5887.85

  15 middleware
  5910.36

  20 middleware
  5019.53

  30 middleware
  4183.37

  50 middleware
  4030.67

  100 middleware
  3978.49
@dougwilson

This comment has been minimized.

Member

dougwilson commented Jul 24, 2014

@vkurchatkin thanks :) though i think @mscdex must have a much better machine than you, as he got this: #2218 (comment)

@dougwilson

This comment has been minimized.

Member

dougwilson commented Jul 24, 2014

Now you know why I had you run them instead of comparing them to his run, because only the same-machine runs are comparable. It may also be a red herring because the promises router from above has nowhere near the features the koa and express have (like mounting URLs, req.params, etc.).

@vkurchatkin

This comment has been minimized.

vkurchatkin commented Jul 24, 2014

@dougwilson sure, that's obvious. I didn't run express benchmarks in the first place because it doesn't support "upstream"

@dougwilson

This comment has been minimized.

Member

dougwilson commented Jul 24, 2014

it doesn't support "upstream"

sure, but neither does the promises one above. the classic "upstream" example is https://github.com/koajs/koa/blob/master/docs/guide.md which will explode on the setHeader with headers already sent error if you tried to do that with promise-kernel...?

@dougwilson

This comment has been minimized.

Member

dougwilson commented Jul 24, 2014

The most meaningful feedback everyone can give us is use the promise-kernel module already published on npm and rewrite your real live web servers using it and give us feedback for how it works, geeze.

@mscdex

This comment has been minimized.

mscdex commented Jul 24, 2014

@dougwilson Actually my benchmarks were run on an (older) Core i7-860 machine, 8GB RAM, running Ubuntu 14.04. I get faster results on my Haswell-based ultrabook (at least ~4k more requests for 1-30 middleware benchmarks or so).

@gtomitsuka

This comment has been minimized.

gtomitsuka commented Jul 17, 2015

i.m.o we can't really have it until nodejs/node-v0.x-archive#7714 is fixed (or at least, promised to be fixed).

Why not Bluebird?

@listepo

This comment has been minimized.

@felixfbecker

This comment has been minimized.

felixfbecker commented Oct 30, 2015

Any update on this?
And btw since no one mentioned it, the .catch() error handler style could be achieved with next() callback style too in theory, by allowing to pass a callback to next().

@calebmer

This comment has been minimized.

calebmer commented Oct 31, 2015

I've been trying to get this merged into https://github.com/pillarjs/router (see #2763 and pillarjs/router#25). Waiting for @dougwilson to review.

@olalonde

This comment has been minimized.

olalonde commented Aug 17, 2016

Any updates on this? Would be nice if middleware could return a promise as an alternative to calling next()/next(err).

Edit: Oops, just noticed it is coming in v5: #2237

@ahmetatar

This comment has been minimized.

ahmetatar commented Mar 31, 2017

Can I take care of it if nobody is working on it?

@dougwilson

This comment has been minimized.

Member

dougwilson commented Mar 31, 2017

There are a few PRs both here and in https://github.com/pillarjs/router . I plan to merge in basic support to 5.0 over the weekend, likely pillarjs/router#32 without the upstream support (for now), since upstream has a lot more kinks to work out.

@q42jaap

This comment has been minimized.

q42jaap commented Apr 6, 2017

In the mean time, for express 4, we have monkey patched Layer to wrap handlers with code that does exactly what we want:
https://gist.github.com/q42jaap/f2fb93d96fda6384d3e3fc51977dec90

@felixfbecker

This comment has been minimized.

felixfbecker commented Apr 6, 2017

We have been using https://www.npmjs.com/package/async-middleware for quite a while now, it just wraps middlewares explicitely without any monkey patching

@xjamundx

This comment has been minimized.

xjamundx commented Sep 20, 2017

Hoping to add something like this into kraken-js krakenjs/kraken-js#495

@JessieAMorris

This comment has been minimized.

JessieAMorris commented Oct 30, 2017

Any word on this? There's a ton of middlewares and such, but this would be nice to get in mainline express.

@wesleytodd

This comment has been minimized.

Member

wesleytodd commented Nov 3, 2017

pillarjs/router#60

It is lined up for the 2.x version of router, which will land in express 5.

@silverwind

This comment has been minimized.

silverwind commented May 30, 2018

Just adding another wrapper option here: https://github.com/Abazhenov/express-async-handler

@affanshahid

This comment has been minimized.

affanshahid commented Aug 29, 2018

Any updates on when promise support will land in Express 5?

@wesleytodd

This comment has been minimized.

Member

wesleytodd commented Aug 29, 2018

The basic support is merged to the router 2.x branch. pillarjs/router#60

There is one other open PR over there, but nce the beta for that is released I think we can release another prerelease version of express 5. I am not sure if @dougwilson has a concrete timeline on that.

@dougwilson

This comment has been minimized.

Member

dougwilson commented Oct 27, 2018

I'm going to close this issue now that Express.js 5.0.0-alpha.7 has been published which includes the initial support for Promises in the router. Middleware and handlers can now return promises and if the promise is rejected, next(err) will be called with err being the value of the rejection. The implementation is seeking feedback from real usage, and please open any feedback as a new issue, either in this issue tracker or in the router issue tracker.

I am currently working on writing up Express.js-specific documentation on this feature, but in the meantime, the documentation can be found in the router repository:

https://github.com/pillarjs/router/tree/v2.0.0-alpha.1#middleware

The function can optionally return a Promise object. If a Promise object is returned from the function, the router will attach an onRejected callback using .then. If the promise is rejected, next will be called with the rejected value, or an error if the value is falsy.

@dougwilson dougwilson closed this Oct 27, 2018

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