Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor async #11

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

eschwartz
Copy link

This pull request includes no functional changes. However, it does drop support for Node 0.x.

I have refactored the code using co to better control the flow of asynchronous code.

My primary motivation for using co is to improve error handling. In callback-style asynchronous code, it's really easy to swallow exceptions. For example:

function info(callback) {
  fs.readFile(this.filename, 'utf8', (err, data) => {
    if (err) { return callback(err); }

    // This will throw an uncaught exception, for invalid yaml
    const info = yaml.load(data);

    // This will throw an uncaught exception if `info = undefined`
    const styles = info.styles.map(() => { ... });
    ...
    return data;
  })
}

Neither of the errors above are passed to callback, which means that whoever is calling info cannot catch them.

You can wrap everything in try/catch blocks, but this starts to get really messy for deep-nested code.

I looked at using async, which has the advantage of supporting Node 0.x, and it's already a dependency. However, it comes with some of the same problems, and some new ones.

Consider:

function info(callback) {
  async.waterfall([
    // Errors from readFile are passed to callback (async)
    next => fs.readFile(this.filename, 'utf8', next),
    (data, next) => {
      // But this still throws an uncaught exception, for invalid yaml (sync)
      const info = yaml.load(data);

      async.map(info.styles, (filename, next) => {
        // Still room for uncaught errors here, too...
        fs.readFile(path.join(path.dirname(this.filename), filename), (err, mss) => {
          if (err) { return next(err); }

          return {filename, mss}
        });
      }, next);
    },
    (styles, next) => {
      // Dealing with scope / closures gets really messy here too.
      // I need access to `info`, which was defined in the previous function.
      // I could pass it on via a `result` object from the last function,
      // but you can see how this style of code can become hard to reason about.
      info.styles = styles.reduce(...);
    }
  ])
}

co, on the other hand, treats async and sync errors the same, so you don't need to worry about uncaught exceptions. It also allows you to keep everything in the same scope, making reasoning about your code much easier.

function info(callback) {
  return co(function* () {
    // If this.filename does not exist, the error will be passed to `callback` (async)
    const data = yield cb => fs.readFile(this.filename, 'utf8', cb);

    // If the yaml if invalid, the error will be passed to `callback` (sync)
    const info = yaml.load(data);

    ... 
    return info;
  })
    .then(info => callback(null, info), err => callback(err));
}

The syntax is very similar to the proposed async/await syntax

async function info(callback) {
  const data = await fs.readFile(this.filename, 'utf8');
  ...
}

As I noted in pull request #10, I'm trying to debug timeouts in my tile server, and I'm suspecting that some of the timeouts are caused by uncaught exceptions in upstream code (including tilelive-tmstyle). I'm hoping these changes will allow me to catch, log, and fix some of these errors.

Edan Schwartz added 12 commits September 7, 2016 12:09
previously this would cause an uncaughtException,
because the TypeError was not passed to the callback
was not resetting before each test
- prevent uncaught exceptions
  (co treats sync and async exceptions the same)
- improve readability (escape from callback hell)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant