Add Promise API (Bonus: finally support)

[CrabDude]

An opt-in Promise API is the best avenue for adding finally support.

Likely, the callback API will be deprecated at a point that makes sense and avoids as much inconvenience as possible for the community

[electrotype]

Hi,

Do I understand correctly that trycatch is currently unable to catch errors occuring in promises?

For example, I try to use it on an Express route :

app.get("/test", function (req, res, next) {

    trycatch(function () {
        
        // This works, the error handle is called!
        //process.nextTick(function () {
        //    throw new Error("monkey wrench");
        //});

        // This does not work!
        Promise.resolve().then(() => {
            throw new Error("some error!");
        });

    }, function (err) {
        next(err);
    });
});

Do you know of a way to globally catch any async errors, even those occuring inside promises?

I try to not use the global unhandledRejection handler since I want to return a "500" error as the response when an error occures.

Thanks in advance!

[CrabDude]

It's actually totally possible, but a little more invasive than try catch already is. Effectively, you would need to track all Errors in Error.prepareStackTrace, and then filter for unhandledRejection errors. You could implement this fairly easily by slightly augmenting an existing userspace implementation of onUnhandledRejection, but instead of emitting the error on the process.onUnhandledRejection event, you would just pass it to the current context's active catch call back / handler (when it exists) instead. This wouldn't really add any additional functionality that is not already provided via the process.onUnhandledRejection event and long stack traces, except the ability to catch in context. It would be like a request (500) level unhandledRejection. That is something that would be nice to have.

[electrotype]

First thanks for your reply!

I'm quite new to Node.js so I have to admit I don't understand everything you say, sadly.

You could implement this fairly easily by slightly augmenting an existing userspace implementation of onUnhandledRejection,

How can I do that? Something like tweaking the process prototype itself? By extending the process object?

Or are you talking about writing code inside the global handler (we use TypeScript, by the way) :

process.on('unhandledRejection', (reason: any, p: any) => {
    // ...
}

Because, in this handler, I'm not sure how I could pass anything "to the current context's active catch call back / handler (when it exists)". How can I make it return a 500 status code to the HTTP request that triggered the error, but not to the other requests?

Are you able to provide some quick code snippets to get me started? This is the first time I have to implement error management in an async framework, and I find it quite difficult. Thanks again!

[CrabDude]

This would be implemented in lib/trycatch.js.

Off the top of my head, the way unhandledRejection works presently is that for every promise exception, it adds an asynchronous check for whether the rejection was handled synchronously. At that point, instead of process.emit('unhandledRejection', error), you would need to process.domain.emit('error', error), which should automatically get routed to the relevant catch handler.

It's not immediately obvious how to accomplish the above without mucking with / coupling the implementation to node.js internals (e.g., promise.js). However, we can take advantage of a few things and create poor-man's version of this for the time being, that should be sufficient:

1. EventEmitter.prototype.emit is synchronous and serial.
2. trycatch already wraps/shims & catches errors thrown from EventEmitter handlers.
3. EventEmitter.prototype.emit ignores setting the active domain for process event handlers.
4. trycatch is mean to be the first require in your process to properly shim core APIs.

Given the above, merely adding the following should/might be sufficient:

// process.domain.emit cannot be passed directly b/c it changes dynamically
process.on('unhandledRejection', function(e) {
    // This will be caught be the active domain...
    // ... And avoids emitting on other unhandledRejection handlers
    // Active domain error handler is the relevant trycatch catch function
    if (process.domain && !process.domain._disposed) throw e
})

// Run the above just before the hookit call that wrap core...
// ... to avoid wrapping the handler (and mucking with the active domain)
// ... Though given the special EventEmitter logic for process, this probably doesn't matter

// Pass callback wrapping function to hookit
hookit(wrap)

You could verify this by adding a test that does the following:

var err = new Error('This should get passed to catchFn')

trycatch(function catchFn() {
    setImmediate(function() {
        // Create a rejected promise that has no reject handler
        Promise.reject(err)
    })
}, function catchFn(e) {
    assert.equals(err, e)
})

The existing test suite (npm test) is fairly comprehensive, but there would be several other tests needed to fully cover this new functionality. Off the top of my head:

1. catchFn should be async
2. Long stack traces are as expected
3. process.on('unhandledRejection', ...) & process.on('uncaughtException', ...) do not fire.
4. Ensure pre-existing domain (set prior to trycatch(...) does not interfere.
5. Ensure numerous peer & nested trycatch calls work as expected & do not interfere.

Finally, this will only work for promise implementations that properly emit process.on('unhandledRejection', ...), and so will be incompatible with any homegrown solutions that do not do so. That said, it seems to be getting standardized / convention-ized, so it will likely be sufficient, and won't break anything.

Let me know if that works. Thanks for taking the time to work on this. Happy to help as best I can given my limited availability.

[ChrisCinelli]

I have always used this middleware in express:

app.use((req, res, next) => trycatch(next, next));

The default error handler is able in this way to find what happened and provide a nice colorized long stack trace with the detail of the error. This has saved me a lot of time in debugging.

Lately promises with no .catch() have been a problem.
The best I was able to to do has been:

global.Promise = require('bluebird');
Promise.config({
  longStackTraces: true
});

But I would still like to see trycatch work with promises and being able to use the default error handler in express.

What can I do to help?

[ChrisCinelli]

BTW, after a few attempts this works with the problem that I currently have:

global.Promise = require('bluebird');
Promise.config({
  longStackTraces: true
});

process.on('unhandledRejection', function(e) {
    if (process.domain && !process.domain._disposed) throw e
});
var trycatch = require('trycatch');
trycatch.configure({ 'long-stack-traces': true });

// ...

app.use((req, res, next) => trycatch(next, next));

It also seems to work on native promises.

Just make sure you add:

process.on('unhandledRejection', function(e) {
    if (process.domain && !process.domain._disposed) throw e
});

Before:

var trycatch = require('trycatch');

However, the long stack trace is not completed!