From dd8c57f0312fde9a99ca57f765dc24daff009994 Mon Sep 17 00:00:00 2001 From: Blake Embrey Date: Fri, 4 Dec 2015 17:10:42 -0800 Subject: [PATCH] Handle promise errors in router middleware * Add lots of `return` calls for proxying the function returns back to the previous middleware stack function * Catch promise errors and pass to `next` --- index.js | 34 +++++++++++++--------------------- lib/layer.js | 22 ++++++++++++++++++---- lib/route.js | 6 +++--- package.json | 1 + test/router.js | 34 ++++++++++++++++++++++++++++++++++ test/support/utils.js | 2 +- 6 files changed, 70 insertions(+), 29 deletions(-) diff --git a/index.js b/index.js index b488e58..87f8e05 100644 --- a/index.js +++ b/index.js @@ -28,11 +28,6 @@ var setPrototypeOf = require('setprototypeof') var slice = Array.prototype.slice -/* istanbul ignore next */ -var defer = typeof setImmediate === 'function' - ? setImmediate - : function(fn){ process.nextTick(fn.bind.apply(fn, arguments)) } - /** * Expose `Router`. */ @@ -61,7 +56,7 @@ function Router(options) { var opts = options || {} function router(req, res, next) { - router.handle(req, res, next) + return router.handle(req, res, next) } // inherit from the correct prototype @@ -186,7 +181,7 @@ Router.prototype.handle = function handle(req, res, callback) { req.baseUrl = parentUrl req.originalUrl = req.originalUrl || req.url - next() + return next() function next(err) { var layerError = err === 'route' @@ -208,8 +203,7 @@ Router.prototype.handle = function handle(req, res, callback) { // no more matching layers if (idx >= stack.length) { - defer(done, layerError) - return + return done(layerError) } // get pathname of request @@ -281,7 +275,7 @@ Router.prototype.handle = function handle(req, res, callback) { var layerPath = layer.path // this should be done for the layer - self.process_params(layer, paramcalled, req, res, function (err) { + return self.process_params(layer, paramcalled, req, res, function (err) { if (err) { return next(layerError || err) } @@ -290,7 +284,7 @@ Router.prototype.handle = function handle(req, res, callback) { return layer.handle_request(req, res, next) } - trim_prefix(layer, layerError, layerPath, path) + return trim_prefix(layer, layerError, layerPath, path) }) } @@ -298,8 +292,7 @@ Router.prototype.handle = function handle(req, res, callback) { var c = path[layerPath.length] if (c && c !== '/') { - next(layerError) - return + return next(layerError) } // Trim off the part of the url that matches the route @@ -324,9 +317,9 @@ Router.prototype.handle = function handle(req, res, callback) { debug('%s %s : %s', layer.name, layerPath, req.originalUrl) if (layerError) { - layer.handle_error(layerError, req, res, next) + return layer.handle_error(layerError, req, res, next) } else { - layer.handle_request(req, res, next) + return layer.handle_request(req, res, next) } } } @@ -399,7 +392,7 @@ Router.prototype.process_params = function process_params(layer, called, req, re value: paramVal } - paramCallback() + return paramCallback() } // single param callbacks @@ -412,20 +405,19 @@ Router.prototype.process_params = function process_params(layer, called, req, re if (err) { // store error paramCalled.error = err - param(err) - return + return param(err) } if (!fn) return param() try { - fn(req, res, paramCallback, paramVal, key.name) + return fn(req, res, paramCallback, paramVal, key.name) } catch (e) { - paramCallback(e) + return paramCallback(e) } } - param() + return param() } /** diff --git a/lib/layer.js b/lib/layer.js index 99b25d2..0ab617d 100644 --- a/lib/layer.js +++ b/lib/layer.js @@ -15,6 +15,20 @@ var pathRegexp = require('path-to-regexp') var debug = require('debug')('router:layer') +/** + * Normalize the middleware result with promise error handling. + */ + +function normalize (result, next) { + if (result && typeof result.then === 'function') { + return result.then(null, function (err) { + return next(err || new Error('Promise was rejected with a falsy value')) + }) + } + + return result +} + /** * Module variables. * @private @@ -66,9 +80,9 @@ Layer.prototype.handle_error = function handle_error(error, req, res, next) { } try { - fn(error, req, res, next) + return normalize(fn(error, req, res, next), next) } catch (err) { - next(err) + return next(err) } } @@ -90,9 +104,9 @@ Layer.prototype.handle_request = function handle(req, res, next) { } try { - fn(req, res, next) + return normalize(fn(req, res, next), next) } catch (err) { - next(err) + return next(err) } } diff --git a/lib/route.js b/lib/route.js index 8b67f2d..ba92f59 100644 --- a/lib/route.js +++ b/lib/route.js @@ -106,7 +106,7 @@ Route.prototype.dispatch = function dispatch(req, res, done) { req.route = this - next() + return next() function next(err) { if (err && err === 'route') { @@ -133,9 +133,9 @@ Route.prototype.dispatch = function dispatch(req, res, done) { } if (err) { - layer.handle_error(err, req, res, next) + return layer.handle_error(err, req, res, next) } else { - layer.handle_request(req, res, next) + return layer.handle_request(req, res, next) } } } diff --git a/package.json b/package.json index bbb37bd..60ca8aa 100644 --- a/package.json +++ b/package.json @@ -16,6 +16,7 @@ }, "devDependencies": { "after": "0.8.1", + "bluebird": "3.0.6", "finalhandler": "0.4.0", "istanbul": "0.4.0", "mocha": "2.3.3", diff --git a/test/router.js b/test/router.js index d174185..e94b864 100644 --- a/test/router.js +++ b/test/router.js @@ -1,6 +1,7 @@ var after = require('after') var methods = require('methods') +var Bluebird = require('bluebird') var Router = require('..') var utils = require('./support/utils') @@ -539,6 +540,24 @@ describe('Router', function () { .expect(404, done) }) + it('should read downstream results', function (done) { + var router = new Router() + var server = createServer(router) + var message = 'hello world' + + router.use(function (req, res, next) { + return next().then(res.end.bind(res)) + }) + + router.use(function (req, res, next) { + return Bluebird.resolve(message) + }) + + request(server) + .get('/') + .expect(200, message, done) + }) + describe('error handling', function () { it('should invoke error function after next(err)', function (done) { var router = new Router() @@ -570,6 +589,21 @@ describe('Router', function () { .expect(200, 'saw Error: boom!', done) }) + it('should invoke error function after rejected promise', function (done) { + var router = new Router() + var server = createServer(router) + + router.use(function handle(req, res, next) { + return Bluebird.reject(new Error('boom!')) + }) + + router.use(sawError) + + request(server) + .get('/') + .expect(200, 'saw Error: boom!', done) + }) + it('should not invoke error functions above function', function (done) { var router = new Router() var server = createServer(router) diff --git a/test/support/utils.js b/test/support/utils.js index 8d115f4..8080962 100644 --- a/test/support/utils.js +++ b/test/support/utils.js @@ -16,7 +16,7 @@ function createHitHandle(num) { var name = 'x-fn-' + String(num) return function hit(req, res, next) { res.setHeader(name, 'hit') - next() + return next() } }