2

I just started working on a new project and no one throws exceptions over here. I get that you shouldn't be doing that for asynchronous code, but it seems to be a valid way to do things for the synchronous side of things.

I would typically do something like this:

function(thing, cb) {
  async_funct(thing, function(stuff) {
    let other_stuff = sync_thing(stuff)
    try {
      let more_other_stuff = other_sync_thing(other_stuff)
    } catch(err) {
      cb(err)
    }
    cb(null, err)
  })
}

Where as in the project it would be treated like this:

function(thing, cb) {
  async_funct(thing, function(stuff) {
    let other_stuff = sync_thing(stuff)

    let more_other_stuff = other_sync_thing(other_stuff)
    if(more_other_stuff instanceof Error)
      cb(err)

    cb(null, err)
  })
}

Is this a standard thing? I'm interested to know how others are handling this type of thing and why.

2
  • 1
    unrelated to your question, but most JS shops use camelCase for variable names and such rather_than_underscore_separating_words.
    – Paul
    Jun 8, 2017 at 22:49
  • The big difference I see between indicating an error by throwing an exception and by returning an exception is that throwing an exception makes things easier. Don't indicate errors by returning exceptions unless you have a specific, practical reason to do that. Jun 8, 2017 at 23:49

4 Answers 4

1

While Neil's answer is pretty accurate, things may change in the future with the advent of async functions in ECMAScript 8. Support for async functions was added in Node 7.6 and Node 8 (LTS). In addition, Node 8 added the util.promisify function which opens up much potential.

Throwing an error inside an async function is the equivalent of rejecting a promise. Instead of having multiple different ways of handling errors (catch for synchronous functions, (err, ...args) for callbacks, and .catch() for promises), asynchronous code will look synchronous and the try/catch construct can be used.

For example:

Using promises, handling async errors with .catch and sync errors with try/catch:

const promisify = require('util').promisify
const fs = require('fs')
const readFileAsync = promisify(fs.readFile)

readFileAsync('foo.txt', {encoding: 'utf8'})
  .then(data => {
    console.log('CONTENT:', data)

    try {
      let moreOtherStuff = syncThing(data)
      return Promise.resolve(otherSyncThing(moreOtherStuff))
    } catch (err) {
      return Promise.reject(err)
    }
  })
  .catch(err => console.error(err))

Using async functions, handling errors with try/catch:

const promisify = require('util').promisify
const fs = require('fs')
const readFileAsync = promisify(fs.readFile)

(async () => {
  try {
    let data = await readFileAsync('foo.txt', {encoding: 'utf8'})
    console.log('CONTENT:', data)
    let moreOtherStuff = syncThing(data)
    return otherSyncThing(moreOtherStuff)
  } catch (err) {
    console.error(err)
    throw new Error(err)
  }
})()

As you can see, the code uses the same try/catch block to handle the error (whether or not this is desirable depends on the use case and personal preference). Regardless, the code looks more synchronous when read top-down.

In addition to all of this, try/catch/finally statements could not be optimized prior to V8 5.3 (the JavaScript engine used in Node 7.x) so performance could be a factor. However this should no longer be the case going forward, as the engine is now supposedly able to optimize those blocks.

2

This type of thing is not standard, at least not in the projects that I've worked in.

My guess is that the exceptions alter the otherwise normal flow of the code as is. Returning an error is, in a way, like surrounding every call with a try catch. If an error were thrown, then it may potentially bypass code which needs to be run prior to returning an error.

Granted, this is not best practice. You should be throwing and catching exceptions if they are generally exceptions, and the code should be properly prepared to deal with these exceptions wherever a try catch is present in the call stack. Asynchronous functions are a little more tricky to deal with errors, but certainly not impossible.

In your case, there may not be a difference in behavior, though as these things generally go, if you ask a project manager if a major change can be made to a project that could potentially insert bugs that goes against the general accepted norm of the project in question for the scope of no added functionality or benefit other than being structured a little better, the answer is usually no. ;)

2

The way you say you would typically do it is how I would do it as well, and how most of the node core libraries expect you to do it.

Synchronous code can and will throw exceptions, while async code will either be event emitters (in which case you have to listen to the 'error' event) or have a callback (in which case it's an error-first pattern) or be Promise-based (which have .then() and .catch()).

In all cases except the callback one (sadly), failing to catch an error (either b/c you're not listening for the 'error' event or don't use .catch() to deal with a revoked promise, or dont' use try..catch will result in the application exiting with an error code at runtime.

It is definitely the correct pattern to do something like:

function myFunc(input, callback) {
    let bob = null;
    try{ 
      bob = someSyncFn();
    } catch (e) {
      return callback(e);
    }
    return callback(null, bob);
}

Though, conceptually if you're able to handle the error in the catch statement (for example, if the operation is adding additional data, and you don't really care if it's added or not) then you could swallow that error, though I rarely do in practice. At a minimum, I'll log a warning if I do that.

The other case that's a little different is that in Express, I'll have an error handler that is responsible for reporting sanitized error comments to the client. That error handler (a middleware) will log the real error, res.send() a 'cleaned' error (complete with http status code) to the client, and then swallow the error so that the app doesn't crash.

0

In JavaScript, you don't have compiler checked exceptions, so there aren't protections in place to make sure callers handle exceptions. For that reason I think it's better practice to return an object that is either an error or a result. That way you force the caller to address the potential for error. This should be properly documented and obvious to the caller. I don't think it's very obvious in your second code snippet because it involves checking the return type. Something like Either from monet.js is, in my opinion, a better solution.

Example (using monet.js):

/**
 * @return {Either[Error,Int]} A random number or an error if the random was zero
 */
function other_sync_thing() {
    const rand = Math.random()
    if (rand === 0) {
        Either.Left(new Error("random crash"))
    }
    return Either.Right(rand)
}

function(thing, cb) {
  async_funct(thing, function(stuff) {
    let other_stuff = sync_thing(stuff)
    // We can either catch and log the error like so:
    other_sync_thing(other_stuff).cata(err => console.err(err), result => cb(result))
    // Or we can bubble the either up to the callback for it to handle like so:
    cb(other_sync_thing(other_stuff))
  }
} 
9
  • 1
    Returning an object that might be an error or a result is an antipattern, regardless of how good your documentation is.
    – Paul
    Jun 8, 2017 at 23:05
  • @Paul Can you provide evidence for that claim? I am interested in the rationale.
    – Samuel
    Jun 8, 2017 at 23:06
  • Well to be fair, most of the sources I found easily are coming from C# and Java, but I don't think the lack of a compiler checking for you changes things. The basic reasoning is that you're depending on calling code to know that they should be checking for error conditions on return values, while thrown exceptions cause an application to crash rather than proceed in a possible error state.
    – Paul
    Jun 8, 2017 at 23:22
  • This puts more cognitive load on future people maintaining your code, which is never a good thing for long term maintainability.
    – Paul
    Jun 8, 2017 at 23:23
  • 1
    I'm not suggesting checking the response to see if its an error. I'm suggesting that calling code will have to handle both cases because the return type demands it. With monet's Either, caller must map (to bubble the exception) or cata (to catch and handle the exception). There is no checking involved. monet.github.io/monet.js/#either
    – Samuel
    Jun 8, 2017 at 23:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.