throw error in catch block Q promise not rejecting the promise

Question

I have the following function which is called by another function:

 function fetchAllRecords(client, item, region, offset, savedCount, totalCount) {
  offset = offset || 1;
  savedCount = savedCount || 0;
  totalCount = totalCount || 0;
  return processQueue(client, item, offset, region).then(function (result) {
    return associate(result, item, region)
  }).then(function (success) {
    return saveBatch(item, success.allResources, region);
  }).then(function (result) {
    savedCount += result.savedCount;
    totalCount += result.totalCount;
    if (debugmode) {
      console.log(savedCount + '/' + totalCount);
    }
    offset += item.limit;
    return fetchAllRecords(client, item, region, offset, savedCount, totalCount);
  }).catch(function (err) {
    if (err == 'done')
      return 'done';

    // All of the above steps have built-in retry so we assume there's a non-recoverable error in this batch and move to next
    if (err.message === 'LIMIT_EXCEEDED') {
      offset += item.limit;
      return fetchAllRecords(client, item, region, offset, savedCount, totalCount);
    }
    else {
******************* Problem Line ********************
      console.log('Miscellenious error in fetachAllRecords, moving to next resource');
      console.log(JSON.stringify(err));
      throw new Error('Misc');
    }
  });
}

and it is being called like this in the other function

function processResource(client, item, debugmode, region) {
  var deferred = q.defer();
  if (item.resource === "Property" && region === "ccmls") {
    var cities = cities.list;
    item.query = item.query + ' ,(City=|' + cities.join() + ')';
  }
  fetchAllRecords(client, item, region)
    .then(function (result) {
      if (debugmode) {
        console.log('fetchAllRecords: ' + item.resource + '.' + item.class + ' completed...');
      }
      deferred.resolve(result);
    })
    .catch(function (err) {
      console.log(item.resource + '.' + item.class + ' failed with error: ' + JSON.stringify(err));
      deferred.reject(err);
    });
  return deferred.promise;
}

In the above problem line, it is supposed to reject fetchAllRecords and in processResource the fetachAllResources catch handler should be called, but for some weird reason, the problem line keeps being called after throw and after getting called a dozen times(at random) it finally rejects the promise returned by fetchAllResources in processResource. Am I missing something obvious here? Also please comment on my style of using promises, is it alright or I need more practice?

Avoid the [deferred antipattern](http://stackoverflow.com/q/23803743/1048572)! — Bergi, Dec 11 '16 at 14:41

David E · Answer 1 · 2016-12-11T17:31:48.397

I think it's likely that the error you receive happens, say, after you have a dozen calls to your method on the stack.

Ie, imagine you have the following scenario when calling the processQueue methods:

Success, Success, Success, Misc Fail

Now, observe the lines I've marked in the code below. (I will refer to the lines as, eg LINE A2, which will refer to LINE A in the second call of fetchAllRecords):

function fetchAllRecords(client, item, region, offset, savedCount, totalCount) {
  offset = offset || 1;
  savedCount = savedCount || 0;
  totalCount = totalCount || 0;
/*********************** LINE A **************************/
  return processQueue(client, item, offset, region).then(function (result) {
    return associate(result, item, region)
  }).then(function (success) {
    return saveBatch(item, success.allResources, region);
  }).then(function (result) {
    savedCount += result.savedCount;
    totalCount += result.totalCount;
    if (debugmode) {
      console.log(savedCount + '/' + totalCount);
    }
    offset += item.limit;
/*********************** LINE B **************************/
    return fetchAllRecords(client, item, region, offset, savedCount, totalCount);
  }).catch(function (err) {
    if (err == 'done')
      return 'done';

    // All of the above steps have built-in retry so we assume there's a non-recoverable error in this batch and move to next
    if (err.message === 'LIMIT_EXCEEDED') {
      offset += item.limit;
      return fetchAllRecords(client, item, region, offset, savedCount, totalCount);
    }
    else {
/*********************** LINE C **************************/
      console.log('Miscellenious error in fetachAllRecords, moving to next resource');
      console.log(JSON.stringify(err));
/*********************** LINE D **************************/
      throw new Error('Misc');
    }
  });
}

What happens as we enter in is:

LINE A1 // Completes successfully, proceeds through thens
LINE B1
LINE A2 // Completes successfully, proceeds through thens
LINE B2
LINE A3 // Completes successfully, proceeds through thens
LINE B3
LINE A4 // ERRORS... the exception bubbles
LINE B3 // Exception is bubbling up...
Q service -> detects an exception in the then block contains the line B3, redirects to catch... Let's assume it's a miscellaneous error
LINE C3
LINE D3 // ERRORS...
Q service -> detects an exception in the catch block containing line D3... That is, the promise created on Line B2 is rejected. But as this promise the is returned, it forms part of the linear promise chain in method 2. Thus this rejection is propogated onto the catch... so we next hit:
LINE C2
LINE D2 // ERRORS...
repeat.
LINE C1
LINE D1 // ERRORS...
Q service propogates the rejection back into processResource which then hits the catch block there.

This results in line D being called many more times than the catch block in process resource.

Hope that makes sense.

RE your use of promises - you use an antipattern in the processResource method (reference: https://github.com/petkaantonov/bluebird/wiki/Promise-anti-patterns#the-deferred-anti-pattern ) - you should very rarely have to create deferreds yourself, instead, you can rely on the chaining behaviour of then and catch (see https://github.com/kriskowal/q ). Ie, you could write something like:

function processResource(client, item, debugmode, region) {
  if (item.resource === "Property" && region === "ccmls") {
    var cities = cities.list;
    item.query = item.query + ' ,(City=|' + cities.join() + ')';
  }
  return fetchAllRecords(client, item, region)
    .then(function (result) {
      if (debugmode) {
        console.log('fetchAllRecords: ' + item.resource + '.' + item.class + ' completed...');
      }
      return result;
    })
    .catch(function (err) {
      console.log(item.resource + '.' + item.class + ' failed with error: ' + JSON.stringify(err));
      return Q.reject(err);
    });
}

On a more general note, if you have the ability to a transpiler, I'd recommend using something like babel (or typescript) - it means you can write with ES6 arrow function notation, which can make promises a lot more readable.

No problem. Thanks for identifying the problem, I've posted a solution to avoid the multiple logging — Bergi, Dec 11 '16 at 17:29
I like your explanation - and it's a nice fix you suggested there — David E, Dec 11 '16 at 17:34

score 1 · Accepted Answer · edited May 23 '17 at 12:13

You're getting a multitude of logs because you use a recursive approach, and handle and rethrow the error on every level. Written synchronously, it's exactly the same as

function fetchAll(offset) {
    if (offset > 5) throw new Error("message"); // let's say the inner one throws
    try {
        return fetchAll(offset+1);
    } catch(e) {
        console.error(e.message);
        throw e;
    }
}
fetchAll(0);

You'd expect to get 5 messages here as well, right?

The solution is not to handle errors from the inner results again. To achieve this with promises, have a look at the difference between .then(…).catch(…) and .then(…, …) - you want the latter:

function fetchAllRecords(client, item, region, offset=1, savedCount=0, totalCount=0) {
  return processQueue(client, item, offset, region).then(function (result) {
    return associate(result, item, region)
  }).then(function (success) {
    return saveBatch(item, success.allResources, region);
  }).then(function (result) {
    savedCount += result.savedCount;
    totalCount += result.totalCount;
    if (debugmode) {
      console.log(savedCount + '/' + totalCount);
    }
    offset += item.limit;
    return fetchAllRecords(client, item, region, offset, savedCount, totalCount);
  }, function (err) {
// ^^
    if (err == 'done')
      return 'done';

    if (err.message === 'LIMIT_EXCEEDED') {
      offset += item.limit;
      return fetchAllRecords(client, item, region, offset, savedCount, totalCount);
    } else {
      console.log('Miscellenious error in fetchAllRecords, moving to next resource');
      console.log(JSON.stringify(err));
      throw new Error('Misc');
    }
  });
}
function processResource(client, item, debugmode, region) {
  if (item.resource === "Property" && region === "ccmls") {
    var cities = cities.list;
    item.query = item.query + ' ,(City=|' + cities.join() + ')';
  }
  return fetchAllRecords(client, item, region)
  .then(function (result) {
    if (debugmode) {
      console.log('fetchAllRecords: ' + item.resource + '.' + item.class + ' completed...');
    }
    return result;
  }, function (err) {
    console.log(item.resource + '.' + item.class + ' failed with error: ' + JSON.stringify(err));
    throw err;
  });
}

throw error in catch block Q promise not rejecting the promise

2 Answers2