configurable error handling

[Raynos]

Is it possible to turn try { ... } catch off in the bluebird promise implementation ?

It would be nice if we could opt out of having the then handler wrapped in a try catch.

This would enable people to make different decisions about error handling opinions instead of going with the "defaults" promises/A+ choose.

[petkaantonov]

That would probably be in a separate build like the zalgo build.

[Raynos]

I actually imagine i would require src/promise manually and pass in opts to create my own Promise constructor instead of having a build i can use.

[domenic]

class PromiseThatMissesThePointOfPromises {
  then(onSomething, onSomethingElse) {
    return super(
      value => try {
        return onSomething(value);
      } catch (e) { setImmediate(() => throw e); },
      reason => try {
        return onSomethingElse(reason);
      } catch (e) { setImmediate(() => throw e); }
    );
  }
}

[stefanpenner]

@domenic what have you done, put it back!

[Raynos]

@domenic it doesn't miss the point.

try catch is too dangerous. it assumes all my code is throw safe

[stefanpenner]

-1, fighting the wrong fight.

[Raynos]

What ever error handling mechanism you decide is the best in your JavaScript applications is irrelevant. It would be nice if there was a promise implementation that allowed you to pick one. I don't care to argue about which is best.

i.e. not being opinionated in bluebird would allow more folks to use promises.

[petkaantonov]

Adding such flexibility in the main build to support this at runtime has tangible downsides like implementation complexity and even though minimal, performance loss to everyone else to support a feature nobody in their right mind would use.

There must be something you are missing - can you describe the use case that would require this, and provide example code?

If by throw safe, you mean exception safety, then it doesn't assume your code is exception safe.

Like if you have:

.then(...)
.then(...)
.then(() => {
   createLotsOfState();
   JSON.parse(userInput);
   clearLotsOfState();
});
.then(...)

Then that is actually wrong. Of course try catching that handler is worse than letting it crash the process. However both are shit
because the latter enables someone keep your server down with very little resources. That situation is terrifying enough even when you can fight back.

You would not write any state clearance code inside the try clause like you are doing in the above. All state clearance goes into finally (or you can also implement something like C#'s using, see bottom) and that just works. And it works in promises too:

.then(...)
.then(...)
.then(() => {
   createLotsOfState();
   JSON.parse(userInput);
});
.then(...)
.finally(function(){
    clearLotsOfState();
});

Bluebird implements features when there is tangible benefit to real world code. Being opionated is a completely arbitrary quality and can be used to describe literally anything.

I know you love C# so here is a wonderful pattern

using (Font font1 = new Font("Arial", 10.0f),
            font2 = new Font("Comic Sans MS", 72.0f)) 
{
    byte charset = font1.GdiCharSet;
}

function disposer(resource) {
    resource.dispose();
}

function using() {
    var resources = [].slice.call(arguments, 0, arguments.length - 1);
    if (resources.length < 1) throw new TypeError();
    var fn = arguments[arguments.length - 1];
    if (typeof fn !== "function") throw new TypeError();

    var promiseForInspections = Promise.settle(resources);

    promiseForInspections.then(function(inspections) {
        return inspections.map(function(inspection) {
            if (inspection.isRejected()) {
                throw inspection.error();
            }
            return inspection.value();
        });
    }).spread(fn).finally(function() {
        //.value() never throws here
        ASSERT(promiseForInspections.isFulfilled());
        var inspections = promiseForInspections.value();
        inspections.forEach(function(inspection) {
            if (inspection.isFulfilled()) {
                var resource = inspection.value();
                try {
                    disposer(resource);
                }
                catch (e) {
                    //Not sure what to do here
                    //at the very least we should keep closing the rest of the resources
                    //but we cannot change the rejection reason because this is in .finally
                }
            }
    });

}


using(
    Font.loadAsync("Arial", 10.0),
    Font.loadAsync("Comic Sans MS", 72.0), function(font1, font2) {
    var charset = font1.getGdiCharSet(); 
});

[spion]

@Raynos even if you're not catching errors, its still better if bluebird does. In development mode it will attach extra stack traces to them, helping you figure out what led to the error happening.

Many people are over-blowing the dangers of catching thrown errors. Yes, they exist. No, they're not nearly as prevalent as it looks in typical code. They only apply in very specific situations - when you're catching errors of code that modifies non-local mutable state. But this can also be done safely:

function stuff() {
  return db.begin(tx => {
    var localMutableState = 1;
    return executeOneThing(tx)
    .then(_ => doSomethingElse(tx) && ++localMutableState)
    .then(_ => fetchThing(tx)) 
    .then(thing => transform(thingg.typo))
  });
}

Transaction autorollback on error; connection is autoreleased to the pool, all thanks to promises doing try-catch and giving me Promise.prototype.finally which allows me to implement db.begin

And since I did not catch any unexpected exceptions (the typo is unexpected) with bluebird the error logged to stderr by default.

I didn't have to do anything at all with local mutable state. The GC took care of that.

Another great thing: I can now call stuff() in the context of other code without worrying about cleanup.

As long as you're modifying non-local mutable state within using blocks, you're completely safe. Turns out, this is doable with 99% of my typical code. Your use case may be different -- I'd love to hear more. Also, nonlocal mutable state is fairly obvious in 90% of the cases, and 87% of statistics are made up on the spot.

So throw is not at fault here. Mutable state is. You can fubar mutable state even without throw:

doThingsThatMutateState(function(err, res) {
  if (err) return callback(err); // forgot cleanup
  // continue
});

Oops, did not clean up. Resources are leaking. State is fubar. Just like with promises when you forget to use a using block -- except you must clean-up manually instead of the using block invoking the correct cleanup code for every resource. With promises, the cleanup is much simpler.

The blog post izs wrote is very specific to domains and node core. Domains capture everything, affecting core and library internals, where who knows who keeps who knows what kind of mutable state. That is indeed very dangerous, and promises do not and cannot do that.

What izs downplayed is the cost of restarting an entire node process, and the vulnerability of the entire thing. If someone finds a way to trigger a crashing throw on your service/website, they can easily keep it down -- unless you have very sophisticated and complex machinery to defend against it (e.g. autoblocking IP addresses wont help because of proxies, but autoblocking user accounts mignt) and that machinery isn't completely safe either.

edit: and now I see that @petkaantonov alredy wrote a similar answer. oops.

[spion]

@petkaantonov unfortunately that first finally example is also potentially dangerous -- if one of the functions before createLotsOfState(); fails (or if it fails in the middle) the finally block will attempt to clearLotsOfState();.

• If the cleanup is done by completely discarding and recreating the affected state, it will be okay. This is easiest to get right and it simulates a "process crash" without the downsides.
• If its done by disposing of multiple things one by one (like e.g. sockets) the cleanup itself may crash.
  • If the order of the clean-up doesn't match the order of the resource allocations, resources will be leaked.
• If the cleanup routine does't assume that any single thing was successfully allocated or mutated, it will be okay.

Its still slightly dangerous.

Same goes inside the using routine. Since allocation is done asynchronously, you might end up with an array of resources that looks like this:

[unallocated, allocated, allocated]

We can't use Promise.all() there, we need allSettled. Otherwise .all will fail without cleaning up the successfully allocated resources.

Once we do that, value() will throw a type error on the first resource that was not successfully allocated. Which means that the first resource.inspect().value().dispose(); will fail, leaving the last 2 resources allocated (unless forEach continues even when a call fails).

var invokeSafe = function(fnattr) { 
  return function() {
    var args = [].slice.call(arguments, 1);
    return Promise.bind(arguments[0]).try(function() { 
      return this[fnattr].apply(this, args);
    });
  };
}

// and then replacing forEach with
resources.then(resources => resources.map(invokeSafe('dispose')))

Finally is a low-level primitive -- I recommend against using it directly, as its easy to get it wrong... (I got it wrong 3 times while writing using, and it still needs more work) But once you implement using with it, it becomes a non-issue.

Its also better to implement using for a single resource, then on top of that build using for multiple resources.

[petkaantonov]

A clean up routine will not cause any problems if there is nothing to clean up. Can you come up with even one reasonable example where clean up routine is not possible if there is nothing to clean up? For example, removeAllListeners will do nothing if there is no listeners to remove so there is no harm in calling it. Closing a database connection method will not do anything if there is no connection and so on.

Yes, allSettled should be used and you also cannot call value().dispose() like that. We should take time to come up with the ultimate using method (since it's so hard to get right) and provide it as an extension.

[Raynos]

Since any piece of code could throw in javascript I would have to have constructors & destructors for every side effect. Every piece of memory creation, every file descriptor, every network request.

Nobody does that in node. There is no culture of writing throw safe code. You either write throw safe code or never throw errors. Luckily there is a strong culture of never throwing errors in node. I would much rather not throw ever and manually return errors using an Either interface whenever something actually goes wrong.

The problem with crashing processes is simple. You have to do it unless your code is 100% throw safe. The idea of me writing 100% throw safe code sounds scary complex. I can't imagine how i would do that without going massively into OCD defensive programming.

Btw the JSON.parse example is silly. Of course I have a safeJSONParse that I use instead.

[Raynos]

As an aside, the argument that I should let bluebird catch errors to add meta data (stacktraces) and then rethrow them is a good enough argument for try catch.

[petkaantonov]

If you have went as far as wrapping synchronous functions in an awkward api to avoid using errors in your code I don't understand what you would gain from handlers not using try catch if they never throw.

Yes, you would use using with every resource, javascript doesn't get a break here. Although allocated memory is freed automatically so you don't need anything for that.

[spion]

@Raynos there is no such no-throw culture in node. Core throws all the time. Functions in core throw every time they encounter an invalid argument. Libraries often do that too.

If anything, there is a culture of writing code by ignoring the possibility of any errors. Just look at code samples involving streams. Most have no error handling whatsoever.

Also, if such a strong culture existed, we wouldn't be having this discussion, as it would've been of little importance whether Bluebird catches thrown errors or not if nobody actually threw those errors.

[spion]

However - I'm currently beginning to study functional programming in more detail, and I'd indeed like to learn more about how you do your error handling and cleanup with the Either monad.

To me, so far promises seem to behave very similarly to the Either monad (ignoring recursive thenable assimilation). The only difference is that they can hold multiple kinds of Left (i.e. Exception<Type>), and they automatically create a Left i.e. Exception<Type> value when an exception is thrown from within a callback passed to .then (sort of having tryAny on everything)

Here are a couple of examples - a toy example and a chatroom, and I'm curious to learn how you would handle them.

Toy example: Connect to two resources, read something from one, write that thing to the other, and make sure everything is closed in all situations after the function finishes (most importantly, that the socket handles are relased)

With petkaantonov's modified using (after maybe adding settle() on all promises returned by dispose())

function readWrite() {
  return using(connectToSrc(), connectToDest(), (src, dst) => 
    src.readData().then(withThis(dst, dst.writeData))); 
}

connectToSrc() or connectToDest() may fail, and each allocates a connection. They both have a .dispose() method. using automatically invokes dispose() once the promise inside the third callback argument is settled (fulfilled with a value or rejected with an error).

Example 2: cleaning up an entire chatroom with lots of mutable state

You don't have to do the whole cleanup manually. Just pick a complete object of mutable state which you know how to destroy and recreate. Lets say, an entire chatroom.

acquire (getChatroom('name'), room => {
  extraStuff = room.mutilateCompletely();
  var otherThings = extraStuff.typoo.invoke()
  room.finishMutilating();
  return otherThings;
});

acquire is a version of using that only invokes dispose on the object if an error occurs. Basically, it guarantees that either the entire callback will execute, or the room will be disposed.

To make this work, getChatroom(name) would always create a new chatroom with that name if that chatroom doesn't exist -- or the existing one has been disposed.

var getChatroom = (name) => {
  var room = dictionary.get(name)
  if (room && !room.disposed) return room;
  else return dictionary.put(name, createChatroom(name));
}

The chatroom has a dispose method that marks it disposed and disposes of all its sockets

chatroom.dispose = () => {
  this.disposed = true;
  // ohh, russian-doll disposal and RAII :D
  this.sockets.map(invokeSafe('dispose')); 
}

Now the best thing happens. Once the room is removed from the dictionary and the sockets are disposed, obviously all clients will disconnect. But the next client that attempts to acquire the chatroom will cause its recreation. The old chatroom may have had lots of internal mutable state which cannot be guaranteed to be usable after an error, but one thing is certain - we can always recreate the entire thing, since we already have a constructor for it.

The old room will not be accessed anymore by new clients.

Its conceptually very similar to crashing a process -- but without the downsides of denial of service. And it doesn't require that much work - we just need to get rid of the file handles (non-memory resources) manually. The GC does the rest of the job. (I imagine C++ users would likely say, like, kids this days have it easy! Why back in my day we had to do this for all memory too! And we had to do it in a super-complex, super-ugly language with an unbelieveable number of quirks pertaining to exceptions! and yeah, thats pretty insane).

If you ask me, this definitely beats crashing an entire process with potentially thousands of chatrooms...

Maybe we should expand this to an app of some sort, showcasing the different approaches?

[spion]

Oh, and I forgot to address one remaining point, which I found in the agen-spec faq section

• node.js core shares the same call stack as application code
• node.js core always executes at the top of all call stacks
• node.js is highly asynchronous, even internally
• Because of the above, uncaught exceptions in application code prevent the core call stack from unwinding, placing it into an undefined state, requiring a restart of the process
• The required restart on uncaught exception is a DoS liability, and must therefore be mitigated by leaving exceptions for truly exceptional circumstances

Everything is great about this, except for the hopelessness of the conclusion. The complete conclusion is "must be mitigated by either throwing nothing, or catching everything"

So yeah, core (or another library) calls our callback, which becomes the top of the stack:

cb(null, results)

With promises, this is actually a wrapper for our real callback, which looks like this:

function cb(err, res) {
  if (err) findAndScheduleCatchHandlerSafely(err)
  else  { 
    var err = scheduleSafely(realCallback, result);
    if (err is error) findAndScheduleCatchHandlerSafely(err);
  }
}

Which means that promises isolate our callbacks from directly throwing to node core. They don't let us interrupt the unwinding -- whatever callback we provide, they create a safe version of that callback that catches all exceptions we might throw and never actually let core or external libraries see them. Then they use the promise error propagation mechanism (which is very similar to the one found in the Either monad) to dispatch all those errors to the appropriate error handler.

So here, catching everything is definitely the right thing to do.

And if a library is that badly designed that it gets into an unusable or leaky state after throwing synchronously (i.e. not in the callbacks, but immediately after you invoke it), then you should either consider not using that library or wrapping it with a crashy wrapper

function throwCrash(e) { 
  process.nextTick(function() {
    throw e;
  });
}

function unsafeWrapper(unsafeFn, lib) {
  return function() {
    var d = Promise.defer(),
        args = [].slice.call(arguments).concat([d.callback]),
        self = lib || this;      
    try { 
      return unsafeFn.apply(self, args);
    } catch (e) {
      throwCrash(e);
    }
  }
}
lib.fnAsync = unsafeWrapper(lib.fn);

[Raynos]

@spion catching everything is really hard.

A lot of modules are async and have callbacks you can't access or wrap or make safe. It might be interesting to see if you can use async-listener to fix this and make all internal callbacks used by node core & npm modules 100% "catch everything" safe.

[Raynos]

@spion also never throwing is still simpler then catching everything.

[stefanpenner]

@Raynos you may have just discovered why promises have this guarantee.

[Raynos]

@stefanpenner if the author of the module using promises never uses setTimeout or process.nextTick or any non promisified apis.

Even then the implementation of the browser HOST environment and the implementation of node core itself could still throw asynchronously.

[Raynos]

Also just because promises handle errors doesn't mean throwing is ok.

If you throw in the listener of .on('foo', function () {}) for any event emitter, any event target, any jquery event object, any backbone event object then you probably broke their code.

There is a lot of code that looks like

function foo() {
  // set up state
  emitter.emit('bar')
  // clean up state
}

function foo assumes that run to completion semantics are held. if you throw in the listener for 'bar' then foo is an undefined state.

The idea that throwing is sometimes ok and sometimes not ok sounds like a bad idea.

[spion]

How exactly are you going to throw there?

blah.on('foo', function() {
  // if it throws, the process will crash
  someFunction() 
    // if it throws, it will just bubble up through promises *but* cleanup will run
    .then(_ => safeToThrow()) 
    // same as above
    .then(_ => safeToThrow()) 
});

Alternatively

blah.on('foo', function() {
  // if any of these throws, it will bubble up 
  // through promises and the cleanup will run
  Promise.try(someFunction) 
    .then(_ => safeToThrow()) 
    .then(_ => safeToThrow()) 
});

So basically with promises, to the outside world it looks like never throwing, but on the inside we know that its catching everything.

In general though, event emitters really suck.

Maybe promise-streams (work in progress)

submitStream(form).pipe(ps.through(data => {
  // throw all you like here...
  return searchQuery(data).then(displayResults));
}));

Or a nice FRP library based on promises, which could probably do something like this

Streams or FRP patterns are much better, and in those cases

iterate(_ => waitNextSubmit(form))
  .map(data => searchQuery(data).then(displayResults));

[Raynos]

@spion so basically the rule is only throw inside promise chains ?

Everything outside of promise land is 100% no throwing ?

Which means only writing throw safe code inside promise chains.

[spion]

Yea, inside callbacks passed to .then - or if you want to start safely, Bluebird gives you Promise.try

You will either get a process crash (if you don't wrap the initial call with Promise.try), or run-to-completion of the event handler. You cant get undefined state of other code (of the event emitter that called you).

Its just like wrapping each of your callbacks with try { } catch {} so that the emitter can invoke them safely -- but easier, because .then will do it for you. In contrast, domains will wrap every single thing with try catch, including code that wasn't designed to run that way.

(Same problem applies to async listener)

[Raynos]

@spion but if I were to write event emitter code and were to call .emit() without wrapping it in try catch im not writing throw safe code.

Which means I'm assuming the listener never throws. So either the listener never throws or always catches. Does that mean never throwing and always catching are the same thing ?

[spion]

Yeah, from the perspective of outside code, they are -- the event-emitter doesn't care whether you've wrapped everything with try-catch or you never throw, it all looks the same to it.

[Raynos]

This was a very informative thread for me. Thanks, I think I'm fine with try catch usage.

[wprater]

@spion how have your promise-streams progressed? Im looking into an issue where I have an async queue streaming image downloads to the fs and want to be sure I can correctly handle any stream errors. Sounds like your library could work well.

[spion]

@wprater Its here - https://github.com/spion/promise-streams - but these dayse I recommend looking at reimplementations of node streams (instead of extensions like promise-streams). Node streams are too broken and quirky to fix.