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

Handle promises in middleware (catch rejections) #32

Closed

Conversation

blakeembrey
Copy link
Member

I need more time to focus on the problem and backward compatibility and what it all should mean. This is just some code and tests that I wrote in a few hours, but got stuck with upstream propagation of errors because the models conflict. Here's a couple of conclusions from the little test:

  1. The simplest thing to support with minimal changes is just rejected promise handling, but I also want the ability to have upstream propagation (a la Koa)
  2. The upstream propagation model is conflicting with the current model which is entirely sequential
  3. If upstream propagation were to exist, it would probably not be compatible with current error middleware handlers - this might not be bad, but it's probably a drawback

If someone else has tried something similar, please share your own conclusions and whether I should keep moving forward with trying to support upstream propagation of promise errors (currently I have upstream propagation of resolved promises working fine).

Example:

router.use(async function (req, res, next) {
  try {
    await next()
  } catch (err) {
    renderError(err)
  }
})

router.use(function (req, res, next) {
  return Promise.reject(new Error('boom!'))
})

router.use(function (err, res, res, next) {
  // This should work, but if an error occurs it'll propagate back up before exiting.
})

Full support will also restrict the node version, I believe, to >= 0.12, unless we want to include a polyfill in the library.

Edit: So the piece I'm stuck most on and trying to resolve the best way to support is any legacy code that does not return (aka breaks the promise chain).

Edit 2: Probably best to drop all this extra attempt at handling upstream and move it to a separate router module. I'm having trouble reconciling how to support both modes at once, probably not (easily) possible.

* Add lots of `return` calls for proxying the function returns back to the previous middleware stack function
* Catch promise errors and pass to `next`
@blakeembrey
Copy link
Member Author

@dougwilson I brought down the scope as discussed, and removed the "upstream" handling. This should be a separate module, async-router, if #29 lands.

@blakeembrey blakeembrey changed the title WIP: First-class promise handling Handle promises in middleware (catch rejections) Dec 7, 2015
@calebmer
Copy link

I also tried this and was planning to make a pull request after my other two (#25 and #26) were merged. You can find that code here and the specific commit here if you want to compare. I can't remember all my conclusions (it was so long ago) but I do remember the problem of deciding when to call done to (complete the router middleware) and when to resolve the promise. Both actions need to be completed, but which goes first? Which goes second? I ended up deferring the final done call, but that was probably not the best approach.

@calebmer
Copy link

When I was developing the error catching feature, I ran into this concern. Is this also a concern in your implementation?

I'd be very excited to see this merged, I've been using my implementation calebmer/router for a while now waiting on #26 😊

If you want some extra tests from when I did this see this compare.

@blakeembrey
Copy link
Member Author

@calebmer No, it's not. I mentioned a few times in your previous PR not to mix the concerns, since they weren't related, so I've not included it here. I was looking at the compare, but it looks like most of the changes are related to next double handling, which is in your other PR anyway.

Aside from that, I think there's a flaw (just briefly reading through) in the logic of your next promise. It's unrelated to this PR, but I'm just bringing it up with you in case you haven't considered it, but your "upstream" is resolving in the same order as the original stack (not in reverse) and the stack is not chained (haven't tested). The other issue with deferring like you did, is that next ends up propagating in two directions. The call to done will propagate the chain forward, while your promise will resolve in reverse. You'll run into some race conditions with that.

Anyway, yeah, run into the same concerns too. I did this same change a year ago with the same conclusion too, IIRC. I think proper upstream handling should instead be a part of a separate router implementation (probably async-router), which relies on #29 existing before I can build it out (theoretically).

@calebmer
Copy link

Yeah for the reasons you stated I did abandon the idea as well, however error catching is still crucial.

One note though, they I built the system there is one promise per request. My goal was more focused on catching errors then providing a good upstreaming system. Not much thought was put into it so I did not consider some of those ideas.

@frogcjn
Copy link

frogcjn commented Jun 25, 2016

Any progress? @calebmer, @blakeembrey

@seungha-kim
Copy link

why is this pending so long?

@dougwilson dougwilson mentioned this pull request Oct 14, 2017
7 tasks
@wesleytodd
Copy link
Member

To follow up on this, can we drop the bluebird dep and then get this merged?

@dougwilson
Copy link
Contributor

Which PR were we going to go with, #32 or #47 ?

@wesleytodd
Copy link
Member

Hm, I am not clear on the differences. Let me dig in a bit more.

@dougwilson
Copy link
Contributor

I believe on my work machine I have one of those two staged after resolving conflicts. I won't be back to work until later this week, but I would think that would be the one. (I believe that is the machine I was working on the 2.0 branch on).

@wesleytodd
Copy link
Member

Ok, I think the other one is a smaller change, and should solve the problem at hand. So that seems the most "promising" ;) to me

@dougwilson
Copy link
Contributor

The plan is to release an 2.0 alpha 1 with the promise changes so we have time to play with them as well, so can change our minds after the fact too.

@wesleytodd
Copy link
Member

Ok, so I am wondering what all these returns are for here? Is this supposed to be used to await the next call? I am wondering what a use case for this would be? Because if there is a good one, then maybe this one is better.

@blakeembrey
Copy link
Member Author

Just a heads up, but I think promise handling (basically the code from this PR in normalize) should be merged. I think any upstream handling could be attempted in a future release (probably backward compatibly too). I'd recommend a new PR instead of this one.

To @wesleytodd's point, yes, the returns were to ensure compatibility with upstream handling (you could await next() to know when the upstream is finished assuming every upstream returned a promise). I moved on a bit from using this router to simpler middleware behaviour like https://github.com/serviejs/throwback but the use-case is the same. You can do things such as catching any downstream errors and handling them, or make it easier to implement things such as timers:

router.all(function (req, res, next) {
  const startTime = Date.now() // Actually use `process.hrtime` in reality.
  const result = await next()
  res.setHeader('X-Timer', Date.now() - startTime)
  return result
})

@blakeembrey
Copy link
Member Author

blakeembrey commented Apr 1, 2018

So basically, #47 looks solid today. If someone wants to look into the advanced functionality in the future, that's cool, but I probably won't be myself since keeping backward compatibility here is difficult to do and "downstream/upstream" behaviour is probably better as a new router.

@dougwilson
Copy link
Contributor

Yea, #47 is ultimately what is going to end up getting merged into 2.0 as an alpha, at which point I will rebase this PR on top of that so we can evaluate this prior to 2.0 finalization.

@blakeembrey blakeembrey deleted the first-class-promise-support branch May 19, 2021 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants