JavaScript Callback Error Handling

Question

It's common to validate arguments and return error in functions.

However, in JavaScript callback function, such as:

function myFunction(num, callback) {
  if (typeof num !== 'number') return callback(new Error('invalid num'))
  // do something else asynchronously and callback(null, result)
}

I wrote a lot of functions like this, but I wonder if there's something potentially harmful. Because in most cases, the caller assumes this is an asynchronous function and the callback will execute after the code right after the function call. But if some arguments are invalid, the function will immediately call the callback. So the caller must be careful dealing with the situation, that is, an unexpected execution sequence.

I want to hear some advice on this issue. Should I carefully assume that all asynchronous callbacks may be executed immediately? Or I should use something like setTimeout(..., 0) to convert synchronous thing to asynchronous one. Or there's a better solution I don't know. Thanks.

Montevideo

Answer 1

An API should document that it will call the callback either synchronously (like Array#sort) or asynchronously (like Promise#then), and then always obey that documented guarantee. It shouldn't mix-and-match.

So yes, if you have a function that will normally call a callback asynchronously, it should always call it asynchronously, regardless of why it's making the call.

There was a great example in jQuery: When jQuery first added "deferred" objects, they would call the callback synchronously if the deferred had already been settled, but asynchronously if it hadn't. This was the source of a lot of confusion and bugs, which is part of why ES2015's promises guarantee that then and catch callbacks will always be called asynchronously.

---

If possible and not at odds with the rest of the codebase, look at using Promises rather than simple callbacks. Promises provide very clear, simple, guaranteed semantics and composability for asynchronous operations (and interoperation with synchronous ones).

Spokeswoman

I removed my downvote. I guess validation of the way the function is called is severe enough to throw and shouldn't happen in production anyway. – Grumous

@PatrickRoberts: I've removed that part of the answer, it wasn't really relevant to the question. I can see your point, though I don't (yet) agree with it; I need to think rather more about it. I note that if you throw during the setup of an ES2015 Promise (let p = new Promise(resolve => { throw new Error(); })), it gets converted to a rejection by the Promise constructor, which supports your argument in favor of a single error channel given the thought and experience that went into the design of that API... – Spokeswoman

My approach is also incorrect though. As the commenter pointed out to me, the callstack is something very important to consider, especially when a developer using your function attempts to retry indefinitely, ultimately resulting in a stackoverflow if your function is synchronous when it errors. – Grumous

Patrick - Yeah, I do think it's important that async be async and sync be sync. Which argues against my deleted paragraph (and your original answer). – Spokeswoman

Looking back at this, it seems that Promises have gone the way of a single error channel and are staying that way for good. Unfortunately this goes against my philosophy that boneheaded exceptions should be thrown immediately, but since Promises are starting to fix obfuscated stack traces with async/await, I guess it's not such a priority anymore to validate and throw as early as possible. – Grumous

@PatrickRoberts - Standard exception handling also has a single error channel...? You differentiate errors by type (that's a lot easier now we can subclass Error) and/or message. – Spokeswoman

Sorry, I wasn't implying that the standard wasn't already a single error channel, I just meant that the promise implementation and async/await decided not to allow both synchronous and asynchronous throwing of errors from within an asynchronous context. – Grumous

Answer 2

The caller of your asynchronous function should know what's going to be the result of invoking the function. There is a standard for what an asynchronous function should return, Promises.

If your function returns a Promise, anybody can easily understand what's going on in that function. Promises have the reject callback, but we could argue if the validation of the parameters should be handled by rejecting the promise or if an exception should be thrown straight forward. Either way, if the caller handles exceptions properly using the catch method, both directly thrown exceptions and rejects will be captured in the same manner.

function throwingFunction(num) {
  return new Promise(function (resolve, reject) {

    if (typeof num !== 'number') throw new Error('invalid num');
    // do something else asynchronously and callback(null, result)
  };
}

function rejectingFunction(num) {
  return new Promise(function (resolve, reject) {

    if (typeof num !== 'number') reject(new Error('invalid num'));
    // do something else asynchronously and callback(null, result)
  };
}

// Instead of passing the callback, create the promise and provide your callback to the `then` method.

var resultThrowing = throwingFunction(num)
    .then(function (result) { console.log(result); })
    .catch(function (error) { console.log(error); });

var resultRejecting = rejectingFunction(num)
    .then(function (result) { console.log(result); })
    .catch(function (error) { console.log(error); });

Both patterns will result in the error being catched and logged.

If you use promises, the caller of your asynchronous function will not have to worry about your implementation inside the function, and you can either throw the error straight foward or reject the promise as you please.

Schafer

Answer 3

No, calling back immediately is not harmful, and in fact intentionally delaying the error just wastes time and overhead. Yes, calling back immediately for an error can be very harmful and should be a avoided for a function that is assumed to be asynchronous! (Look at that, a 180!)

From a developer's perspective, there are multiple good reasons for why set up can only be done after. For example here:

const server = net.createServer(() => {}).listen(8080);

server.on('listening', () => {});

The listening event isn't attached until after .listen(8080) is invoked, because the event source is returned from the call to .listen(). In this case, implementing the listening event to invoke synchronously after .listen() is executed will be unsuccessful.

Here's another case I'd like to present:

var num = '5';

myFunction(num, function callback(err, result) {
  if (err) {
    return myFunction(num, callback);
  }

  // handle result
});

Now, if you callback with the error synchronously, this control flow will result in a stackoverflow. While this is the developer's fault, a stackoverflow is a really bad thing to occur from a function that's expected to be asynchronous. This is one advantage of using setImmediate() to pass the error instead of executing the callback immediately.

Grumous

Answer 4

I recommend using TypeScript for type check. In you case try the following:

function myFunction(num, callback) {
  return new Promise((resolve, reject)=>{
    if (typeof num !== 'number'){
      reject(new Error('invalid num'))
    }
  })  
  // do something else asynchronously and callback(num, result)
  resolve()
}

Genuflection