New: server strictNext option throws when calling next more than on…

…ce (#1454)

lib/server.js

@@ -253,8 +253,9 @@ function Server(options) {
     this.router = options.router;
     this.routes = {};
     this.secure = false;
-    this.versions = options.versions || options.version || [];
     this.socketio = options.socketio || false;
+    this._once = (options.strictNext === false) ? once : once.strict;
+    this.versions = options.versions || options.version || [];
     this._inflightRequests = 0;
 
     var fmt = mergeFormatters(options.formatters);
@@ -851,9 +852,9 @@ Server.prototype._route = function _route(req, res, name, cb) {
  * 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.
+ * 1) `emittingErrors` - this boolean is set to true when the server is still
+ * emitting error events. 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
@@ -881,11 +882,8 @@ Server.prototype._run = function _run(req, res, route, chain, cb) {
     var log = this.log;
     var self = this;
     var handlerName = null;
-    var emittedError = false;
-
-    if (cb) {
-        cb = once(cb);
-    }
+    var emittingErrors = false;
+    cb = self._once(cb);
 
     // attach a listener for 'close' and 'aborted' events, this will let us set
     // a flag so that we can stop processing the request if the client closes
@@ -914,19 +912,19 @@ Server.prototype._run = function _run(req, res, route, chain, cb) {
         if (typeof arg !== 'undefined') {
             if (arg instanceof Error) {
                 // 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);
-                });
+                // complete this invocation of run() until it returns. set a
+                // flag so that the automatic invocation of cb() at the end of
+                // this function is bypassed.
+                emittingErrors = true;
                 // set the done flag - allows us to stop execution of handler
                 // chain now that an error has occurred.
                 done = true;
+                // now emit error events in serial and async
+                self._emitErrorEvents(req, res, route, arg,
+                function emitErrorsDone() {
+                    res.send(arg);
+                    return cb(arg);
+                });
             } else if (typeof (arg) === 'string') { // GH-193, allow redirect
                 if (req._rstfy_chained_route) {
                     var _e = new errors.InternalError();
@@ -988,7 +986,7 @@ Server.prototype._run = function _run(req, res, route, chain, cb) {
             req._currentHandler = handlerName;
             req.startHandlerTimer(handlerName);
 
-            var n = once(next);
+            var n = self._once(next);
 
             // support ifError only if domains are on
             if (self.handleUncaughtExceptions === true) {
@@ -1019,17 +1017,20 @@ Server.prototype._run = function _run(req, res, route, chain, cb) {
             self.emit('done', req, res, route);
         }
 
-        // 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 {
-            return (true);
+        // if we have reached here, there are no more handlers in the chain, or
+        // we next(err), and we are done with the request. if errors are still
+        // being emitted (due to being async), skip calling cb now, that will
+        // happen after all error events are done being emitted.
+        if (emittingErrors === false) {
+            return cb(arg);
         }
+
+        // don't really need to return anything, returning null to placate
+        // eslint.
+        return null;
     }
 
-    var n1 = once(next);
+    var n1 = self._once(next);
 
     dtrace._rstfy_probes['route-start'].fire(function () {
         return ([
@@ -1104,8 +1105,7 @@ Server.prototype._setupRequest = function _setupRequest(req, res) {
  * @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.
+ * @returns  {undefined}
  */
 Server.prototype._emitErrorEvents =
 function _emitErrorEvents(req, res, route, err, cb) {
@@ -1132,7 +1132,7 @@ function _emitErrorEvents(req, res, route, err, cb) {
     }
 
     // kick off the async listeners
-    vasync.forEachPipeline({
+    return vasync.forEachPipeline({
         inputs: errEvtNames,
         func: function emitError(errEvtName, vasyncCb) {
             self.emit(errEvtName, req, res, err, function emitErrDone() {
@@ -1147,10 +1147,6 @@ function _emitErrorEvents(req, res, route, err, cb) {
         // 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;
 };
 
 

test/server.test.js

@@ -2897,3 +2897,22 @@ test('should emit restifyError even for router errors', function (t) {
         t.done();
     });
 });
+
+
+test('calling next twice should throw', function (t) {
+    SERVER.get('/', function (req, res, next) {
+        res.send(200);
+        next();
+        next();
+    });
+
+    SERVER.on('uncaughtException', function (req, res, route, err) {
+        t.ok(err);
+        t.equal(err.message, 'next shouldn\'t be called more than once');
+        t.end();
+    });
+
+    CLIENT.get('/', function (err, req, res, data) {
+        t.ifError(err);
+    });
+});