Merge pull request #851 from restify/845

Formatter functions now must always return a cb of the type f(err, data)

docs/index.restdown

@@ -412,14 +412,14 @@ in a hash of content-type -> parser at server creation time:
 
     var server = restify.createServer({
       formatters: {
-        'application/foo': function formatFoo(req, res, body) {
+        'application/foo': function formatFoo(req, res, body, cb) {
           if (body instanceof Error)
             return body.stack;
 
           if (Buffer.isBuffer(body))
-            return body.toString('base64');
+            return cb(null, body.toString('base64'));
 
-          return util.inspect(body);
+          return cb(null, util.inspect(body));
         }
       }
     });
@@ -445,14 +445,14 @@ ensure sorting happens the way you want:
 
     restify.createServer({
       formatters: {
-        'application/foo; q=0.9': function formatFoo(req, res, body) {
+        'application/foo; q=0.9': function formatFoo(req, res, body, cb) {
           if (body instanceof Error)
-            return body.stack;
+            return cb(body);
 
           if (Buffer.isBuffer(body))
-            return body.toString('base64');
+            return cb(null, body.toString('base64'));
 
-          return util.inspect(body);
+          return cb(null, util.inspect(body));
         }
       }
     });

lib/formatters/binary.js

@@ -11,9 +11,10 @@
  * @param    {Object} req  the request object
  * @param    {Object} res  the response object
  * @param    {Object} body response body
+ * @param    {Function} cb cb
  * @returns  {Buffer}
  */
-function formatBinary(req, res, body) {
+function formatBinary(req, res, body, cb) {
     if (body instanceof Error) {
         res.statusCode = body.statusCode || 500;
     }
@@ -23,7 +24,7 @@ function formatBinary(req, res, body) {
     }
 
     res.setHeader('Content-Length', body.length);
-    return (body);
+    return cb(null, body);
 }
 
 module.exports = formatBinary;

lib/formatters/json.js

@@ -11,9 +11,10 @@
  * @param    {Object} req  the request object
  * @param    {Object} res  the response object
  * @param    {Object} body response body
+ * @param    {Function} cb cb
  * @returns  {String}
  */
-function formatJSON(req, res, body) {
+function formatJSON(req, res, body, cb) {
     if (body instanceof Error) {
         // snoop for RestError or HttpError, but don't rely on
         // instanceof
@@ -33,7 +34,7 @@ function formatJSON(req, res, body) {
     var data = JSON.stringify(body);
     res.setHeader('Content-Length', Buffer.byteLength(data));
 
-    return (data);
+    return cb(null, data);
 }
 
 module.exports = formatJSON;

lib/formatters/jsonp.js

@@ -11,9 +11,10 @@
  * @param    {Object} req  the request object
  * @param    {Object} res  the response object
  * @param    {Object} body response body
+ * @param    {Function} cb cb
  * @returns  {String}
  */
-function formatJSONP(req, res, body) {
+function formatJSONP(req, res, body, cb) {
     if (!body) {
         res.setHeader('Content-Length', 0);
         return (null);
@@ -33,18 +34,18 @@ function formatJSONP(req, res, body) {
         body = body.toString('base64');
     }
 
-    var cb = req.query.callback || req.query.jsonp;
+    var _cb = req.query.callback || req.query.jsonp;
     var data;
 
-    if (cb) {
-        data = 'typeof ' + cb + ' === \'function\' && ' +
-                cb + '(' + JSON.stringify(body) + ');';
+    if (_cb) {
+        data = 'typeof ' + _cb + ' === \'function\' && ' +
+                _cb + '(' + JSON.stringify(body) + ');';
     } else {
         data = JSON.stringify(body);
     }
 
     res.setHeader('Content-Length', Buffer.byteLength(data));
-    return (data);
+    return cb(null, data);
 }
 
 module.exports = formatJSONP;

lib/formatters/text.js

@@ -11,9 +11,10 @@
  * @param    {Object} req  the request object
  * @param    {Object} res  the response object
  * @param    {Object} body response body
+ * @param    {Function} cb cb
  * @returns  {String}
  */
-function formatText(req, res, body) {
+function formatText(req, res, body, cb) {
     if (body instanceof Error) {
         res.statusCode = body.statusCode || 500;
         body = body.message;
@@ -24,7 +25,7 @@ function formatText(req, res, body) {
     }
 
     res.setHeader('Content-Length', Buffer.byteLength(body));
-    return (body);
+    return cb(null, body);
 }
 
 module.exports = formatText;

lib/response.js

@@ -302,10 +302,21 @@ Response.prototype.send = function send(code, body, headers) {
 
     this._body = body;
 
-    // TODO: restify doesn't ship with any async formatters, but
-    // presumably if an async formatter blows up, we should return a 500.
-    function _cb(err, _body) { // eslint-disable-line handle-callback-err
-        self._data = _body;
+    function _cb(err, _body) {
+        // the problem here is that if the formatter throws an error, we can't
+        // actually format the error again, since the formatter already failed.
+        // So all we can do is send back a 500 with no body, since we don't know
+        // at this point what format to send the error as. Additionally, the current
+        // 'after' event is emitted _before_ we send the response, so there's no way
+        // to re-emit the error here. TODO: clean up 'after' even emitter so we
+        // pick up the error here.
+        if (err) {
+            self._data = null;
+            self.statusCode = 500;
+            log.error(err, 'unable to format response');
+        } else {
+            self._data = _body;
+        }
         Object.keys(headers).forEach(function (k) {
             self.setHeader(k, headers[k]);
         });
@@ -324,11 +335,7 @@ Response.prototype.send = function send(code, body, headers) {
     }
 
     if (body !== undefined) {
-        var ret = this.format(body, _cb);
-
-        if (!(ret instanceof Response)) {
-            _cb(null, ret);
-        }
+        this.format(body, _cb);
     } else {
         _cb(null, null);
     }

test/formatter.test.js

@@ -20,7 +20,7 @@ var PORT = process.env.UNIT_TEST_PORT || 0;
 var CLIENT;
 var SERVER;
 
-var isFirstRequest = true;
+var reqCount = 0;
 
 
 ///--- Tests
@@ -30,21 +30,25 @@ before(function (callback) {
         SERVER = restify.createServer({
             formatters: {
                 'text/plain': function (req, res, body, cb) {
-                    if (isFirstRequest) {
+                    if (reqCount === 0) {
                         process.nextTick(function () {
-                            isFirstRequest = false;
+                            reqCount++;
                             cb(null, 'async fmt');
                         });
-                        return (this);
+                    } else if (reqCount === 1) {
+                        process.nextTick(function () {
+                            reqCount++;
+                            cb(new Error('foobar'), 'async fmt');
+                        });
+                    } else {
+                        return cb(null, 'sync fmt');
                     }
-
-                    return ('sync fmt');
                 },
-                'application/foo; q=0.9': function () {
-                    return ('foo!');
+                'application/foo; q=0.9': function (req, res, body, cb) {
+                    return cb(null, 'foo!');
                 },
-                'application/bar; q=0.1': function () {
-                    return ('bar!');
+                'application/bar; q=0.1': function (req, res, body, cb) {
+                    return cb(null, 'bar!');
                 }
             },
             dtrace: helper.dtrace,
@@ -91,6 +95,21 @@ test('async formatter', function (t) {
     });
 });
 
+test('async formatter error', function (t) {
+    SERVER.once('after', function (req, res, route, e) {
+        // TODO: add a test here to verify error has been emitted.
+        // Pending #845
+    });
+    CLIENT.get('/tmp', function (err, req, res, data) {
+        t.ok(err);
+        t.equal(err.statusCode, 500);
+        t.ok(req);
+        t.ok(res);
+        t.notOk(data);
+        t.end();
+    });
+});
+
 test('sync formatter', function (t) {
     CLIENT.get('/tmp', function (err, req, res, data) {
         t.ifError(err);
@@ -101,7 +120,6 @@ test('sync formatter', function (t) {
     });
 });
 
-
 test('q-val priority', function (t) {
     var opts = {
         path: '/tmp',