Error handling and finally

[Kurt Blackwell]

Howdy,

I'm new to promises, so I'm trying to fit some old forms of error handling into this new pattern, but I'm a bit stumped.

Normal synchronous code would do this:

function run() {
    var db = openDatabase();
    try {
        return doWork(db);
    } finally {
        closeDatabase(db);
    }
}

But my best attempt of achieving the same thing with when.js is this:

function run() {
    var db = openDatabase();
    var result = when(db, doWork);
    return result.always(function() {
        return when(db, closeDatabase)
            .then(function() {
                return result;
            });
    });
}

Not exactly pretty.  If closeDatabase might fail, then I want to know about this, but the first error to occur (in doWork) is more important than errors when closing objects:

function run() {
    var db = openDatabase();
    var result = when(db, doWork);

    return result.always(function() {
        var closed = when(db, closeDatabase);
        return when(closed,
            function() {
                return result;
            },
            function() {
                return result
                    .then(function() {
                        return closed;
                    });
            });
    });
}

Is there some pattern of handling errors in when.js that will achieve what I want without all this complexity?

I've written my own function to make this simpler, but what I would really like is a build-in function to when.js:

function run() {
    var db = openDatabase();
    return when(db, doWork)
        .finally(function() {
            return when(db, closeDatabase);
        });
}

If you have multiple handles to close:

function run() {
    var a = openDatabaseA();
    var b = openDatabaseB(); // Must be a promise, otherwise `a` will leak if `b` throws.
    return when.join(a, b)
        .spread(doWork)
        .finally(function() {
            return when.join(
                when(a, closeDatabase),
                when(b, closeDatabase));
        });
}

finally() is similar to always(), except it would resolve or reject with the value of the promise it's call on.  If it's callback's return value rejects, it would reject on that unless the previous promise also rejected first.

Thoughts?

[Brian Cavalier]

Hey Kurt,

First off, welcome to promises :)  This is a great question, and there are lots of subtleties wrapped up in it!

If I understand correctly, you'd like a construct that allows performing some side effect in the promise chain *without* altering the outcome of the chain.  Hence, if you use .always(f), then f would need to be specifically coded to check for an error and propagate a rejection by rethrowing it, or another returning a rejected promise.

That's definitely an interesting concept.  I'll have to think through it a bit more to see if there are more interesting use cases for it :)  If so, then it's something we can think about adding.

I think the following might be close to what you need.  I wrote it for a single db connection, but it may be possible to extend to multiple connections, like your later examples.

function run() {
    var db = openDatabase();

    // if opening the db fails, this will return a rejection
    return when(db, function(realDb) {


      // Assumption: doWork returns a promise and does not throw
      // if doing work fails, result is a rejection
      var result = doWork(realDb);
     
      // Or if doWork doesn't return a promise. Weird use of when()
      // simply to trap exceptions, but it works:
      //var result = when(realDb, doWork);


      // Always close the database.
      // If it closes successfully, propagate the result, which
      // may be either a success or rejection.
      // If an error occurs while closing, that error will
      // propagate, and the yield will be bypassed
      return result.always(closeDatabase).yield(result);
    });
}

Let me know if that looks like it might work for you!

[Kurt Blackwell]

Ok, that does help thanks Brian.  I seemed to have missed the yield() function, and I'm still a bit cautious when trusting functions to use promises.  It's very hard to be 100% sure doWork won't throw, but maybe with a bit more promise practise I'll think differently.

I would still like something closer to my final example though, where earlier errors take precedence over later ones.  If closeDatabase failed it's likely because of something that happened in doWork.  Although I understand this isn't exactly the same as the basic try...finally.

[Brian Cavalier]

On Wednesday, January 16, 2013 6:28:10 PM UTC-5, Kurt Blackwell wrote:

Ok, that does help thanks Brian.  I seemed to have missed the yield() function, and I'm still a bit cautious when trusting functions to use promises.  It's very hard to be 100% sure doWork won't throw, but maybe with a bit more promise practise I'll think differently.

Yeah, in an ideal world, all promise-returning functions would be guaranteed never to throw an exception.  Since we don't live in an ideal world :), we have to be a bit more careful.  That's why when.js (and most of the "better" implementations) trap exceptions thrown by promise callbacks and transform them into rejected promises.  Using the when(realDb, doWork) in my example will help there.  We'll also have some new features in an upcoming release that will make it easy to call a function, with arguments, in a way that ensures it will always return a promise, and will never throw.

 

I would still like something closer to my final example though, where earlier errors take precedence over later ones.  If closeDatabase failed it's likely because of something that happened in doWork.  Although I understand this isn't exactly the same as the basic try...finally.

Ah, I see.  I misunderstood that you wanted the earlier error to take precedence, sorry about that.  Interestingly, your try/catch example would propagate a later error from closeDatabase() if it happened to throw from within the finally {} :)  Believe it or not, finally can modify the result of a function ... it can even squelch exceptions and return a value.  For example, this will return "win!":

try { throw new Error(); } finally { return 'win!' }

And this will ultimately throw 'b', not 'a':

try { throw 'a'; } finally { throw 'b'; }

Funky :)

Ignoring the later error and propagating an earlier one, while also maintaining the closeDatabase side effect, seems very tricky in synchronous code!  Here's a quick attempt, and it's fairly messy:

function run() {
    var db = openDatabase();

    var result;

    try {
        result = doWork(db);
    } catch(e) {
        // doWork failed, begin exception gymnastics
        try {
            closeDatabase(db);
        } finally {
            // Funky, but works!
            // Squelch the closeDatabase exception, and
            // always rethrow the doWork exception
            throw e;
        }
    }

    // doWork succeeded
    // If closeDatabase throws now, we can let it propagate
    closeDatabase(db);

    // Whew! we made it here, we can actually return a successful result
    return result;
}

It's a bit of a mind-bender :) Is that the effect you are looking for?  I don't know that I've seen that done, but I can see how it could be useful.

So, back to promises.  Here's another attempt.  I think this will prefer earlier errors from doWork over later errors from closeDatabase.  I haven't tried to run it, but I think it's close:

function run() {
    var db = openDatabase();


    // if opening the db fails, this will return a rejection
    return when(db, function(realDb) {

      // Call doWork and trap exceptions:

      var result = when(realDb, doWork);

      // If we know doWork returns a promise and does not throw
      // we can use this instead:
      // var result = doWork(realDb);

      result.always(closeDatabase)
        .yield(result) // closeDatabase succeeded, yield result, which may be success or fail
        .otherwise(function(closeDbError) {
          // closeDatabase failed.
          // If result is success, propagate closeDbError.
          // if result is a failure, the yield will be skipped, so result will propagate.
          return result.yield(closeDbError)
        });
    });
}

Which seems to be about an equal level of messiness as the synchronous version.

Hmmm, while I'm not sure the "prefer the earlier error over a later error" construct is common enough to include it in the when.js core, I can see how some notion of "finally" that is different from "always" might be useful.  It could be used to insert side effects into a promise chain without necessarily changing the outcome of the chain.

Let me know what you think.  This is definitely an interesting case!

 

[Kurt Blackwell]

 

It's a bit of a mind-bender :) Is that the effect you are looking for?  I don't know that I've seen that done, but I can see how it could be useful.

That's kind of what I'm thinking, but you don't need to catch the first error

function run() {

    var db = openDatabase();

    try {

        return doWork(db);

    } finally {

        try {

            closeDatabase(db);

        } catch (e) {

            // Shoo error, don't bother me

        }

    }

}

 

More funky :)

I quite like your second bit of code, though.

However the more I think about this, I'm not sure what I would like.  I cut my programming teeth on C++, and the rule there is that destructors must never, ever, ever throw exceptions because throwing exceptions during an exception will destroy the universe.  A closeDatabase should have it's own non-throw guarantees and just log fatal errors elsewhere.

Is there a good forum for general forum for Promise/A related questions?  I'm going to use when.js in a toy project I just started, but I think I'm going to have lots of questions about general patterns.

[Kurt Blackwell]

Wait...   ignore my code there, it's completely wrong.  I shouldn't write code before bed.

[Brian Cavalier]

On Thursday, January 17, 2013 8:44:28 AM UTC-5, Kurt Blackwell wrote:

I quite like your second bit of code, though.

Thanks!  I think that code is probably a little hard to understand at first glance ... comments would be a necessity! :)

 

However the more I think about this, I'm not sure what I would like.  I cut my programming teeth on C++, and the rule there is that destructors must never, ever, ever throw exceptions because throwing exceptions during an exception will destroy the universe.  A closeDatabase should have it's own non-throw guarantees and just log fatal errors elsewhere.

Yeah, exceptions for closing things or freeing resources are always tricky.  I agree that if the db connection can't be closed, there's just not much that an application-level catch statement could do about it.

 

Is there a good forum for general forum for Promise/A related questions?  I'm going to use when.js in a toy project I just started, but I think I'm going to have lots of questions about general patterns.

This google group is a good spot :)  There is also a new proposal (disclaimer: I am the primary editor) called Promises/A+ that is aiming to improve upon the ideas of Promises/A, and to create new specs around promises.  The group is composed primarily of promise implementors, and there is some good discussion the issues lists of the github repos there.  You could pose questions there by filing issues--we use it for more than just "bugs"--as long as you don't flood us with issues :).  Also, if you pose on twitter and mention "Promises/A+" or "promises-aplus", with a link to a gist, you're likely to get a response from one of the Promises/A+ folks :)

Bring on the promise questions ...

 

[Kurt Blackwell]

Ok, my next question is related to my first, so I'll ask it here...

Why would anyone use .always() ?  It takes both the previous result and rejection error and smushes them into a single argument, so both become indistinguishable and useless.  Any previous error is silently eaten.  I'd be much happier if the callback used the function(err, result) style so I could at least use it to chain to foreign callbacks.

[Brian Cavalier]

On Thursday, January 17, 2013 6:20:10 PM UTC-5, Kurt Blackwell wrote:

Ok, my next question is related to my first, so I'll ask it here...

Why would anyone use .always() ?  It takes both the previous result and rejection error and smushes them into a single argument, so both become indistinguishable and useless.  Any previous error is silently eaten.  I'd be much happier if the callback used the function(err, result) style so I could at least use it to chain to foreign callbacks.

People have generally found .always() to be useful, although I agree it has limited uses.  I don't necessarily agree they are indistinguishable and useless, nor that the error is silently eaten, but again, the use cases might be limited.  Some folks use .always() and test the param to see if it's an error or not, using some simple test that they works given the constraints of their application.

That said, the two arg version you're proposing is interesting, as is the idea of some other construct, similar to .always(), but that allows a side effect to be performed but also passes through the previous result or error and maintains the fulfill/reject state.

Another thought is some sort of construct like .always() that returns undefined instead of a new promise, i.e. so that you *cannot* chain from it and accidentally make the mistake of silencing an error.  I had toyed with this when I was adding .always(), and that's how it worked initially (before it was released), but I changed it to match other promise libraries' behavior.

I'm definitely interested to hear you thoughts on these ideas :)

[Kurt Blackwell]

People have generally found .always() to be useful, although I agree it has limited uses.  I don't necessarily agree they are indistinguishable and useless, nor that the error is silently eaten, but again, the use cases might be limited.  Some folks use .always() and test the param to see if it's an error or not, using some simple test that they works given the constraints of their application.

Most errors in JavaScript are thrown new Error()s, but I thought a promise named it's failure a "reason" because it could be something else explaining the failure?  Is it unusual for a reason to be a string, and the expected value to also be a string?  It's unreasonable to expect cleanup code to tell them apart.

Another thought is some sort of construct like .always() that returns undefined instead of a new promise, i.e. so that you *cannot* chain from it and accidentally make the mistake of silencing an error.  I had toyed with this when I was adding .always(), and that's how it worked initially (before it was released), but I changed it to match other promise libraries' behavior.

There's a few problems here which need solving:

1. Default reporting of errors that reach the end of a chain of promises.
   
2. Allowing custom handling/logging of errors, possibly including chaining to a foreign callback.
3. Reporting unhandled errors that the user might have missed because of an bug when chaining promises.
   
4. finally-block cleanup code.

I don't think .always() currently does any of these properly.  If you give it a proper error parameter this will achieve (2) and (4), assuming the users remembers to re-throw errors they want to continue to propagate.  If you remove the return value, it will solve (1) and (2), but I can't use it for clean-up code anywhere other than the very end of the program.

(3) is very difficult to solve for promises, perhaps impossible.  The only solution I can think of is to set a process.nextTick() timeout whenever an error is caught.  If the error isn't passed to another promise within the cycle, then the default error handler is used.  However this will probably require developers to be particular aware of this behaviour.

[Brian Cavalier]

On Monday, January 21, 2013 9:28:58 AM UTC-5, Kurt Blackwell wrote:

Most errors in JavaScript are thrown new Error()s, but I thought a promise named it's failure a "reason" because it could be something else explaining the failure?  Is it unusual for a reason to be a string, and the expected value to also be a string?  It's unreasonable to expect cleanup code to tell them apart.

Yeah, a reason can be anything, just as you can throw anything.  In practice, people tend to use new Error() for rejections, just as they tend to do the same when throwing.  I think it depends on the situation as to whether it's reasonable or not to be able to tell them apart.  People do it.

 

There's a few problems here which need solving:

1. Default reporting of errors that reach the end of a chain of promises.
   
2. Allowing custom handling/logging of errors, possibly including chaining to a foreign callback.
3. Reporting unhandled errors that the user might have missed because of an bug when chaining promises.
   
4. finally-block cleanup code.

I don't think .always() currently does any of these properly.  If you give it a proper error parameter this will achieve (2) and (4), assuming the users remembers to re-throw errors they want to continue to propagate.  If you remove the return value, it will solve (1) and (2), but I can't use it for clean-up code anywhere other than the very end of the program.

Right, always() was never really intended to solve any of these.  For better or for worse, it was simply meant as a shortcut for then(f, f), and it's a common idiom across many promise libs.  I only really use it at the end of a promise chain--maybe that's a clue that it needs to change :)

For backward compat, I think always() as it exists today will need to stick around, but we could potentially work on devising some new API that solves at least a few of these problems.

I'm not sure what #2 means, could you explain a bit further?  Do you mean allowing a side-effect without changing the value/outcome of the promise chain?

For #4, implementing a true finally-like behavior seems pretty tricky (impossible?) given the behavior of finally that allows a previous return/throw to propagate if a new return/throw is not encountered.  Since promises allow undefined as a legal return value and functions implicitly return undefined, it's impossible (barring function.toString(), of course!) to distinguish between a function that intended to return undefined, and one that intended not to return anything at all.

Maybe if we put some constraints on it, there is something implementable and useful--like always propagating the previous result.  I'm totally making this up, so I have no idea if it's what we'd really want, but maybe it's good starting point for discussion:

sortOfFinally: function(onFulfilledOrRejected) {
    return this.then(
        function(value) {
            try {
                onFulfilledOrRejected();
            } catch(e) {}


            return value;
        },
        function(reason) {
            try {
                onFulfilledOrRejected();
            } catch(e) {}


            throw reason;
        }
    );
}

 

One obvious problem is that it completely swallows all errors and the return value of onFulfilledOrRejected handler.  Does this give you any ideas, though?

(3) is very difficult to solve for promises, perhaps impossible.  The only solution I can think of is to set a process.nextTick() timeout whenever an error is caught.  If the error isn't passed to another promise within the cycle, then the default error handler is used.  However this will probably require developers to be particular aware of this behaviour.

I agree that 3 is impossible to solve without putting some constraints on the problem.  It's basically a halting problem for promises.  We have a when/debug module that handles some, but not all cases.  Using a setTimeout or nextTick to warn in some cases might be a good addition there.  I've been wanting to refactor when/debug for a while (it has lots of little experiments in it), but haven't had time.

Some other promise libs use a done() or end() function to "cap" promise chains explicitly.  This is similar to your idea above about removing the return value from always(), and works reasonably well, but I'm not a fan of it personally.  I don't like adding artificial done/end() calls all over the place, which add refactoring hazards (forgetting to add them, forgetting to remove or move them, etc.).

Thanks for the thoughtful discussion so far!

[Brian Cavalier]

Hey Kurt,

Here's a little experiment with making always() more sane, and a bit more like `finally`.  Basically, it prevents always() from modifying the outcome, unless it rejects (either by throwing or returning a rejection).  So, it can turn a success into a failure, but not turn a failure into a success.  I'm not sure whether that's a good thing yet or not, but figure it might make a good starting point to discuss further.  I don't think always() can solve all the issues you raised, but hopefully we can get closer!

[Brian Cavalier]

I opened a github issue to discuss further.