diff --git a/docs/api/server.md b/docs/api/server.md index 7e0f05488..dd57bec91 100644 --- a/docs/api/server.md +++ b/docs/api/server.md @@ -266,21 +266,6 @@ server.on('restifyError', function(req, res, err, callback) { }); ``` -### FormatterError - -This event is fired when an async formatter returns an error as a result of -calling `res.send()`. Unlike other error events, if you listen this event, it -is expected that you flush a response. Once a formatter has returned an error, -restify cannot make any assumptions about how to format the content. It is up -to you to figure out how to best do that. - -```js -server.on('FormatterError', function(req, res, route, err) { - // err is a formatter error - can't sa - res.end('unsafe to call res.send, in case formatter blows up again!'); -}); -``` - ## after diff --git a/lib/response.js b/lib/response.js index 80d8ebd10..30031719a 100644 --- a/lib/response.js +++ b/lib/response.js @@ -16,8 +16,6 @@ var utils = require('./utils'); ///--- Globals var InternalServerError = errors.InternalServerError; -// make custom error constructors -errors.makeConstructor('FormatterError'); var Response = http.ServerResponse; diff --git a/lib/server.js b/lib/server.js index 6054237a1..397d0fe14 100644 --- a/lib/server.js +++ b/lib/server.js @@ -186,44 +186,11 @@ function ifError(n) { /** - * when an error occurrs, this is used to emit an error to consumers - * via EventEmitter. - * @private - * @function emitRouteError - * @param {Object} server the server object - * @param {Object} req the request object - * @param {Object} res the response object - * @param {Object} err an error object - * @returns {undefined} - */ -function emitRouteError(server, req, res, err) { - - var name; - - if (err.name === 'ResourceNotFoundError') { - name = 'NotFound'; - } else if (err.name === 'InvalidVersionError') { - name = 'VersionNotAllowed'; - } else { - name = err.name.replace(/Error$/, ''); - } - - req.log.trace({name: name, err: err}, 'entering emitRouteError'); - - if (server.listeners(name).length > 0) { - server.emit(name, req, res, err, once(function () { - res.send(err); - server._finishReqResCycle(req, res, null, err); - })); - } else { - res.send(err); - server._finishReqResCycle(req, res, null, err); - } -} - - -/** - * returns true if an error generated is for an options request. + * returns true if the router generated a 404 for an options request. + * + * TODO: this is relevant for CORS only. Should move this out eventually to a + * userland middleware? This also seems a little like overreach, as there is no + * option to opt out of this behavior today. * @private * @function optionsError * @param {Object} err an error object @@ -232,15 +199,32 @@ function emitRouteError(server, req, res, err) { * @returns {Boolean} */ function optionsError(err, req, res) { - var code = err.statusCode; - var ok = false; + return ( + err.statusCode === 404 && + req.method === 'OPTIONS' && + req.url === '*' + ); +} - if (code === 404 && req.method === 'OPTIONS' && req.url === '*') { - res.send(200); - ok = true; - } - return (ok); +/** + * map an Error's .name property into the actual event name that is emitted + * by the restify server object. + * @function + * @private errEvtNameFromError + * @param {Object} err an error object + * @returns {String} an event name to emit + */ +function errEvtNameFromError(err) { + if (err.name === 'ResourceNotFoundError') { + // remap the name for router errors + return 'NotFound'; + } else if (err.name === 'InvalidVersionError') { + // remap the name for router errors + return 'VersionNotAllowed'; + } else { + return err.name.replace(/Error$/, ''); + } } @@ -779,6 +763,7 @@ Server.prototype._handle = function _handle(req, res) { /** * look into the router, find the route object that should match this request. + * if a route cannot be found, fire error events then flush the error out. * @private * @function _route * @param {Object} req the request object @@ -789,33 +774,46 @@ Server.prototype._handle = function _handle(req, res) { */ Server.prototype._route = function _route(req, res, name, cb) { var self = this; + // helper function to, when on router error, emit error events and then + // flush the err. + var errResponse = function errResponse(err) { + return self._emitErrorEvents(req, res, null, err, function () { + res.send(err); + return self._finishReqResCycle(req, res, null, err); + }); + }; if (typeof (name) === 'function') { cb = name; name = null; - this.router.find(req, res, function onRoute(err, route, ctx) { + return this.router.find(req, res, function onRoute(err, route, ctx) { var r = route ? route.name : null; if (err) { + // TODO: if its a 404 for OPTION method (likely a CORS + // preflight), return OK. This should move into userland. if (optionsError(err, req, res)) { - self._finishReqResCycle(req, res, null, err); + res.send(200); + return self._finishReqResCycle(req, res, null, null); } else { - emitRouteError(self, req, res, err); + return errResponse(err); } } else if (!r || !self.routes[r]) { err = new ResourceNotFoundError(req.path()); - emitRouteError(self, res, res, err); + return errResponse(err); } else { - cb(route, ctx); + // else no err, continue + return cb(route, ctx); } }); } else { - this.router.get(name, req, function (err, route, ctx) { + return this.router.get(name, req, function (err, route, ctx) { if (err) { - emitRouteError(self, req, res, err); + return errResponse(err); } else { - cb(route, ctx); + // else no err, continue + return cb(route, ctx); } }); } @@ -823,6 +821,14 @@ Server.prototype._route = function _route(req, res, name, cb) { /* + * `cb()` is called when execution is complete. "completion" can occur when: + * 1) all functions in handler chain have been executed + * 2) users invoke `next(false)`. this indicates the chain should stop + * executing immediately. + * 3) users invoke `next(err)`. this is sugar for calling res.send(err) and + * firing any error events. after error events are done firing, it will also + * stop execution. + * * The goofy checks in next() are to make sure we fire the DTrace * probes after an error might have been sent, as in a handler * return next(new Error) is basically shorthand for sending an @@ -830,10 +836,16 @@ Server.prototype._route = function _route(req, res, name, cb) { * probe (namely so the status codes get updated in the * response). * - * Callers can stop the chain from proceding if they do - * return next(false); This is useful for non-errors, but where - * a response was sent and you don't want the chain to keep - * going. + * there are two important closure variables in this function as a result of + * the way `next()` is currently implemented. `next()` assumes logic is sync, + * and automatically calls cb() when a route is considered complete. however, + * for case #3, emitted error events are async and serial. this means the + * automatic invocation of cb() cannot occur: + * + * 1) `emittedError` - this boolean is set to true when next is called with an + * error. this var is used to avoid automatic invocation of cb(), which is + * delayed until all async error events are fired. + * 2) `done` - when next is invoked with a value of `false`, or handler if * * @private * @function _run @@ -846,7 +858,6 @@ Server.prototype._route = function _route(req, res, name, cb) { * @returns {undefined} */ Server.prototype._run = function _run(req, res, route, chain, cb) { - var d; var i = -1; var id = dtrace.nextId(); req._dtraceId = id; @@ -885,77 +896,27 @@ Server.prototype._run = function _run(req, res, route, chain, cb) { }); function next(arg) { + // default value of done determined by whether or not there is another + // function in the chain and/or if req was not already closed. we will + // consume the value of `done` after dealing with any passed in values + // of `arg`. var done = false; - if (arg) { + if (typeof arg !== 'undefined') { if (arg instanceof Error) { - - // if it's a formatter error, handle it differently. - if (arg.code === 'Formatter') { - // in the case of formatter error, emit a formatterError - // event, which is like an uncaughtException scenario in - // that a response must be flushed by the handler. - res.status(500); - - // if consumer listens to this event, they must flush a - // response or the request will hang. don't fire the event - // unless someone is listening to it. - if (self.listeners('FormatterError').length > 0) { - self.emit('FormatterError', req, res, route, arg); - } else { - // otherwise, log it and send empty response. - log.error(arg, 'error formatting response, ' + - 'sending empty payload!'); - res.end(''); - } - // return early. - return self._finishReqResCycle(req, res, route, arg); - } - - var errName = arg.name.replace(/Error$/, ''); - log.trace({ - err: arg, - errName: errName - }, 'next(err=%s)', (arg.name || 'Error')); - - // always attempt to use the most specific error listener - // possible. fall back on generic 'error' listener if we can't - // find one for the error we got. - var hasErrListeners = false; - var errEvtNames = []; - - // if we have listeners for the specific error - if (self.listeners(errName).length > 0) { - hasErrListeners = true; - errEvtNames.push(errName); - } - // or if we have a generic error listener - if (self.listeners('restifyError').length > 0) { - hasErrListeners = true; - errEvtNames.push('restifyError'); - } - - if (hasErrListeners) { - vasync.forEachPipeline({ - func: function emitError(errEvtName, vasyncCb) { - self.emit(errEvtName, req, res, arg, - function errEvtHandlerDone() { - // the error listener may return arbitrary - // objects, throw them away and continue on. - // don't want vasync to take that error and - // stop. - return vasyncCb(); - }); - }, - inputs: errEvtNames - }, function (err, results) { - res.send(err || arg); - return (cb ? cb(err || arg) : true); - }); - emittedError = true; - } else { + // the emitting of the error events are async, so we can not + // complete this invocation of run() until it returns. calling + // _emitErrorEvents is async, but returns a sync flag. when this + // flag is true, we will avoid automatically calling cb at the + // end of this function, which causes server to move on to the + // next handler in the chain. + emittedError = self._emitErrorEvents(req, res, route, arg, + function emitErrorsDone() { res.send(arg); - } + return (cb ? cb(arg) : true); + }); + // set the done flag - allows us to stop execution of handler + // chain now that an error has occurred. done = true; } else if (typeof (arg) === 'string') { // GH-193, allow redirect if (req._rstfy_chained_route) { @@ -968,7 +929,9 @@ Server.prototype._run = function _run(req, res, route, chain, cb) { return (false); } - // Stop running the rest of this route since we're redirecting + // Stop running the rest of this route since we're redirecting. + // do this instead of setting done since the route technically + // isn't complete yet. return self._route(req, res, arg, function (r, ctx) { req.context = req.params = ctx; req.route = r.spec; @@ -990,13 +953,11 @@ Server.prototype._run = function _run(req, res, route, chain, cb) { } self._run(req, res, r, _chain, cb); }); + } else if (arg === false) { + done = true; } } - if (arg === false) { - done = true; - } - // Fire DTrace done for the previous handler. if ((i + 1) > 0 && chain[i] && !chain[i]._skip) { req.endHandlerTimer(handlerName); @@ -1004,7 +965,6 @@ Server.prototype._run = function _run(req, res, route, chain, cb) { // Run the next handler up if (!done && chain[++i] && !_reqClosed(req)) { - if (chain[i]._skip) { return (next()); } @@ -1028,6 +988,8 @@ Server.prototype._run = function _run(req, res, route, chain, cb) { return (chain[i].call(self, req, res, n)); } + // if we have reached this last section of next(), then we are 'done' + // with this route. dtrace._rstfy_probes['route-done'].fire(function () { return ([ self.name, @@ -1038,6 +1000,8 @@ Server.prototype._run = function _run(req, res, route, chain, cb) { ]); }); + // if there is no route, it's because this is running the `pre` handler + // chain. if (route === null) { self.emit('preDone', req, res); } else { @@ -1046,8 +1010,9 @@ Server.prototype._run = function _run(req, res, route, chain, cb) { self.emit('done', req, res, route); } - // Don't return cb here if we emit an error since we will cb after the - // handler fires. + // Don't return cb here if errors are currently being emitted. they are + // async, so we cannot fire cb until all error events are done + // emitting. if (!emittedError) { return (cb ? cb(arg) : true); } else { @@ -1057,11 +1022,6 @@ Server.prototype._run = function _run(req, res, route, chain, cb) { var n1 = once(next); - // support ifError only if domains are on - if (self.handleUncaughtExceptions === true) { - n1.ifError = ifError(n1); - } - dtrace._rstfy_probes['route-start'].fire(function () { return ([ self.name, @@ -1076,29 +1036,29 @@ Server.prototype._run = function _run(req, res, route, chain, cb) { req.timers = []; if (!self.handleUncaughtExceptions) { - n1(); - return; + return n1(); + } else { + n1.ifError = ifError(n1); + // Add the uncaughtException error handler. + var d = domain.create(); + d.add(req); + d.add(res); + d.on('error', function onError(err) { + if (err._restify_next) { + err._restify_next(err); + } else { + log.trace({err: err}, 'uncaughtException'); + self.emit('uncaughtException', req, res, route, err); + self._finishReqResCycle(req, res, route, err); + } + }); + return d.run(n1); } - - // Add the uncaughtException error handler. - d = domain.create(); - d.add(req); - d.add(res); - d.on('error', function onError(err) { - if (err._restify_next) { - err._restify_next(err); - } else { - log.trace({err: err}, 'uncaughtException'); - self.emit('uncaughtException', req, res, route, err); - self._finishReqResCycle(req, res, route, err); - } - }); - d.run(n1); }; /** - * set up the request by before routing and executing handler chain. + * set up the request before routing and execution of handler chain functions. * @private * @function _setupRequest * @param {Object} req the request object @@ -1125,6 +1085,65 @@ Server.prototype._setupRequest = function _setupRequest(req, res) { }; +/** + * emit error events when errors are encountered either while attempting to + * route the request (via router) or while executing the handler chain. + * @private + * @function _emitErrorEvents + * @param {Object} req the request object + * @param {Object} res the response object + * @param {Object} route the current route, if applicable + * @param {Object} err an error object + * @param {Object} cb callback function + * @returns {Boolean} if error events are firing, which is async, return + * true. else return false. + */ +Server.prototype._emitErrorEvents = +function _emitErrorEvents(req, res, route, err, cb) { + + var self = this; + var errName = errEvtNameFromError(err); + + req.log.trace({ + err: err, + errName: errName + }, 'entering emitErrorEvents', err.name); + + var errEvtNames = []; + + // if we have listeners for the specific error, fire those first. + if (self.listeners(errName).length > 0) { + errEvtNames.push(errName); + } + + // or if we have a generic error listener. always fire generic error event + // listener afterwards. + if (self.listeners('restifyError').length > 0) { + errEvtNames.push('restifyError'); + } + + // kick off the async listeners + vasync.forEachPipeline({ + inputs: errEvtNames, + func: function emitError(errEvtName, vasyncCb) { + self.emit(errEvtName, req, res, err, function emitErrDone() { + // the error listener may return arbitrary objects, throw + // them away and continue on. don't want vasync to take + // that error and stop, we want to emit every single event. + return vasyncCb(); + }); + } + }, function (nullErr, results) { // eslint-disable-line handle-callback-err + // vasync will never return error here. callback with the original + // error to pass it on. + return cb(err); + }); + + // while async listeners are firing, return a boolean indicating whether or + // not we had any listeners to fire. + return (errEvtNames.length > 0) ? true : false; +}; + /** * wrapper method for emitting the after event. this is needed in scenarios diff --git a/test/server.test.js b/test/server.test.js index 61ec8c0bd..7fab37664 100644 --- a/test/server.test.js +++ b/test/server.test.js @@ -2865,3 +2865,35 @@ test('should not emit \'routed\' event on 404', function (t) { t.end(); }); }); + + +test('should emit restifyError even for router errors', function (t) { + + var notFoundFired = false; + var restifyErrFired = false; + + SERVER.once('NotFound', function (req, res, err, cb) { + notFoundFired = true; + t.ok(err); + t.equal(err instanceof Error, true); + t.equal(err.name, 'ResourceNotFoundError'); + return cb(); + }); + + SERVER.once('restifyError', function (req, res, err, cb) { + restifyErrFired = true; + t.ok(err); + t.equal(err instanceof Error, true); + t.equal(err.name, 'ResourceNotFoundError'); + return cb(); + }); + + /*eslint-disable no-shadow*/ + CLIENT.get('/dne', function (err, req, res, data) { + t.ok(err); + t.equal(err.name, 'ResourceNotFoundError'); + t.equal(notFoundFired, true); + t.equal(restifyErrFired, true); + t.done(); + }); +});