Node-restify: Compatibility with promises

[calebmer]

Does restify support catching errors from promises? For example, I'm using babel and async/await functions so most of my middleware functions end up looking like the following:

Server.get('/route', async function (req, res, next) {
  throw new Error('Yikes!')
})

This roughly translates to the following ES5 code:

Server.get('/route', function (req, res, next) {
  return new Promise(function (resolve, reject) {
    reject(new Error('Yikes!'))
  })
})

Does restify catch these errors and output them to the client?

[DonutEspresso]

I don't know how Babel is transpiling them - you'd have to try it out yourself. Re: promises, please see this issue for more information.

[calebmer]

This is literally how babel transpiles it. That's a complex polypill for basically the second code sample above.

The only thing required to implement this feature is basically as follows wherever the handler is called:

var handlerReturn = handler(req, res, next)

if (handlerReturn && typeof handlerReturn.then === 'function') {
  handlerReturn.then(null, next)
}

[calebmer]

If you could point me to the line the route handler is called, I could implement this and submit a PR.

[DonutEspresso]

Unfortunately, implementing support for promises breaks restify's error handling due to semantics of then, which is why I don't think it's something we would add (for now). If you want to use Promises, I'd highly recommend using a wrapper for your handlers, or currying it in a way such that your wrapper unboxes the promise (using done() or a setImmediate()) before calling next.

[calebmer]

Out of curiosity, which semantics?

[DonutEspresso]

I linked to details in my previous comment, but in short throw cannot trigger restify's error handling mechanisms because it's caught by the promise. You'd have to unbox the promise before calling next, in order to ensure that the promise is only handling errors from the current handler, as opposed to errors happening in the fifth or six handler down from where the promise originated.

[calebmer]

I'm not entirely sure you understand the small scope of the feature request. This same proposal was implemented in the express router module. Code delta here (Everything outside lib/Layer.js, is just housekeeping stuff). I hope that helps to potentially clarify things.

The previous issue did not even mention returning the promise and allowing Restify to append its own catch handler. Although this proposal and returning the promise could solve the last issue's problem.

[DonutEspresso]

I'm well aware of the scope of changes involved here. It has various implications in terms of how it alters the error handling behavior of restify. Imagine that I want to use promises, I can do that today even without support in restify, though it means I have to manually call next():

'use strict';

var restify = require('restify');
var server = restify.createServer();

server.on('uncaughtException', function(err, req, res, route) {
    console.error('uncaught exception!');
});

function func1(req, res, next) {
    new Promise(function(resolve, reject) {
        resolve();
    })
    .then(next);
};

function func2(req, res, next) {
    throw new Error('boom');
};

server.get('/', [func1, func2]);
server.listen(3000);

As a consumer, what would you expect the behavior here to be?

[DonutEspresso]

See also restify/node-restify#874 and restify/node-restify#962.

[calebmer]

I would expect the thrown error to be caught by Restify.

I apologize for any offense my comments might have caused. I was just (and still am) very confused at how this is not possible. A look at the source code could really help me understand, but sadly I can't seem to understand how all the parts work together on my own at the moment.

This specific promise issue is a problem I'd like to put some in to address but currently the code is impenetrable to me.

I'm going to close this issue now because you have obviously given this a lot of thought even if I don't understand. A point in the direction of the problem code in this scenario would still be very much appreciated.

[DonutEspresso]

Apologies for sounding curt - my intention was really to get you thinking about the error handling scenarios. It's not that implementing support for invoking then() is difficult (in fact, it's trivial), it's that promises pre-empt domain error handling. That would break restify's existing error handling behavior, thus making this a non starter.

In the previous example, you're right, the expectation is that error _should_ be caught by restify. But it's not. Instead, you'll find that the restify error handler isn't triggered, and the response just hangs. If you add a catch() method to func1 though, you'll find that the error thrown in func2 is being caught by the promise in func1:

function func1(req, res, next) {
    new Promise(function(resolve, reject) {
        resolve();
    })
    .then(next)
    .catch(function(err) {
        // whoa, error thrown in func2 will land you here
    });
};

This happens because the function invoked by a promise's then() is wrapped in a try/catch. This is why func2's thrown error is being caught by func1's promise's catch(). This kind of behavior can cause all sorts of weird and unexpected scenarios when it comes to error handling. FWIW, there's a possibility we may move away from using domains in the future, and were that the case this would become trivial to implement.

I'd highly recommend copying the above snippet and running it locally to help you better understand what's going on.

[calebmer]

Ok, I think I understand, and I'll definetly try to find time to test that out. But basically, when an error gets thrown it propagates up the chain, throwing an error at every next?

But isn't this the same as wrapping one's own code in a try/catch?

[roblav96]

This is how every single one of my routes are on my restify server for my project My Convoy

module.exports = function ( req, res, next ) {

    req.that = {
        data: req.body,
        xid: req.xid
    }

    Promise.resolve().bind( req.that ).then( function () {

        if ( _.isInvalidUndefined( this.data, [ 'message', 'trace', 'err', 'stamp' ] ) ) {
            throw new restify.errors.InternalServerError( 'isInvalidUndefined!' )
        }

        this.data.xid = this.xid
        return r.table( 'errors' ).insert( this.data )

    } ).then( function () {

        res.send( true )
        return next()

    } ).catch( function ( err ) {
        if ( err.statusCode ) {
            return next( err )
        } else {
            return next( new restify.errors.InternalServerError( err ) )
        }
    } )

}

Development using promises this way is incredible.

[avimar]

I also use a pattern similar to that.

Someone just asked about this on gitter... in mocha, you have the option of specifying function(done){} or instead _returning_ a promise, and then mocha can grab the error.
I don't know _how_ that works, but apparently that's possible.

Would that solve the error-swallowing issue?

[JohnieLee]

I asked this on Gitter. What I ended up doing was:

.then(function(result) {
    // do something that throws error
   throw SomeApplicationError()
})
.catch(err) {
  const wrappedError = new WrappedError(err);
  next(err);
}

server.on('Wrapped', function (req, res, err, cb) {
  // Error handler for application error
  // unwrap the error to find actual application error
  cb();
});

This allows me to have the same error handler be used for errors emitted from middleware before the promise as ones coming from within the promise.

What would be a nice feature to have to avoid this is enable subclassing for error handler. I should be able to handle error events on the base error class rather than the error instance.