Nodejs promise all not running as expected

Question

I have a series of promises which I have chained in testCard. This method takes a stripe card number, get the token from stripe and then talks to a third party API which tries to perform purchases with that card. I need to run testCard by looping through an array of card numbers. To do this I have a controller object with a method testAllCards which takes the array of numbers. The array is stored in a config file.

I then run the code from the command line with node cli.js testAllCards.

However when I run it, I get testAllCards has been run before all most promises have resolved. I am obviously missing something here, but can't seem to figure out what it is.

cli.js

const testAllCards = () => {
  return controller.testAllCards(config.get('CARD_NUMBERS'))
    .then((obj) => {
      console.log('testAllCards has been run');
    })
    .catch((e) => {
      console.log('testCards has been run with an error!');
      const _err = new ErrHandler(e, eTopicName, eSnsSubject);
      _err.handle()
        .then(() => {
          console.log('Error has been sent with success to sns');
        });
    });
};

switch(process.argv[2]) {
  case 'testAllCards':
    testAllCards();
    break;
  default:
    console.log('Please run with `testAllCards`');

controller.js

//Tests response from API for different cards
const testCard = (cardNum) => {
  return new Promise((resolve, reject) => {
    const expMonth = new Date().getMonth() + 1;
    const expYear = new Date().getFullYear() + 2;
    const cardObj = {
      cardNum: cardNum,
      expMonth: expMonth,
      expYear: expYear
    };
    let apiCardItem = '';
    return testRequestToApi('getStripeToken', 200, 299, cardObj)
      .then((cardItem) => {
        return testRequestToApi('postNewCard', 200, 299, JSON.parse(cardItem.body));
      })
      .then((apiCard) => {
        apiCardItem = apiCard.body;
        try {
          apiCardItem = JSON.parse(apiCardItem);
        } catch(e) {
          console.log(e);
        }
        return testRequestToApi('sampleAddToCart', 200, 299);
      })
      .then(() => {
        return testRequestToApi('useFailingStripeCards', 400, 499, apiCardItem.id);
      })
      .then(() => {
        return testRequestToApi('deleteCard', 200, 299, apiCardItem.id);
      })
      .then(() => {
        resolve();
      })
      .catch((e) => {
        reject(e);
      });
  });
};

//Loops through the card numbers and runs the test command against them
Controller.testAllCards = (cardsArray) => {
  const items = cardsArray.map((cardNum) => {
    return testCard(cardNum);
  });
  return Promise.all(items);
};

module.exports = Controller;

test-request-to-api.js

'use strict';

const checkStatus = require('./../utils/status-code-checker');
const formHeaders = require('./../utils/form-req-headers');
const request     = require('request');
const expObj = {};

//@requestType {string} - defines which headers and function name to use
//@item {object} - defines item that is being used
expObj.testRequestToApi = (requestType, lowerLimit, upperLimit, item) => {
  return new Promise((resolve, reject) => {
    const reqOps = formHeaders[requestType](item);
    request(reqOps, (err, response, body) => {
      if (err) {
        const badRequest = {
          ErrorMessage: err,
          FuncName: requestType,
          InternalError: true
        };
        return reject(badRequest);
      }
      if (!checkStatus.checkRangeStatusCode(response.statusCode, lowerLimit, upperLimit)) {
        console.log(JSON.stringify(body, null, 2));
        // Set a bad Status error object
        let badStatus = {
          StatusCode: response.statusCode,
          ErrorMessage: body,
          FuncName: requestType,
          InternalError: false
        };
        return reject(badStatus);
      }
      // console.log(response.headers);
      // console.log(body);
      const resObj = {
        headers: response.headers,
        body: body
      };
      // console.log(`******** ${requestType} *********`);
      // console.log(resObj);
      // console.log('----------------------------------');
      return resolve(resObj);
    });
  });
};

module.exports = expObj;

This is the [explicit promise construction anti-pattern](https://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it). This is the first thing you should fix. `new Promise()` is only ever necessary when you are promisifying a callback-based API, which you aren't. In the same go you should switch from `request` to [`request-promise`](https://github.com/request/request-promise). — Tomalak, Jun 14 '17 at 13:14
The next thing you should fix is your reliance on a outer-scope variable (`apiCardItem` in this case) to contain asynchronous state. The return values from your promise handlers ought to contain operation results. — Tomalak, Jun 14 '17 at 13:16
Can you post an answer the summarizes the cause of the error and what you did to fix it, with reference to the sample code from your question? — Tomalak, Jun 14 '17 at 14:17

score 2 · Answer 1 · answered Jun 14 '17 at 14:26

Understanding that new Promise() is used only ever necessary when promisifying a callback based API, changing to request-promise and returning my promises in cli.js solved my issue. The execution flow was correctly maintained in this manner.

Changes to the following files are as followed: cli.js

const testAllCards = () => {
  return controller.testAllCards(config.get('CARD_NUMBERS'))
    .then((obj) => {
      console.log('testAllCards has been run');
    })
    .catch((e) => {
    console.log(e)
      console.log('testCards has been run with an error!');
      const _err = new ErrHandler(e, eTopicName, eSnsSubject);
      return _err.handle()
        .then(() => {
          console.log('Error has been sent with success to sns');
        })
        .catch((e) => {
          console.log('Failed to publish to sns');
          console.log(e);
        });
    });
};

test-request-to-api

'use strict';

const checkStatus = require('./../utils/status-code-checker');
const formHeaders = require('./../utils/form-req-headers');
const rqp         = require('request-promise');
const expObj = {};

//@requestType {string} - defines which headers and function name to use
//@item {object} - defines item that is being used
expObj.testRequestToApi = (requestType, lowerLimit, upperLimit, item) => {
  const reqOps = formHeaders[requestType](item);
  return rqp(reqOps)
    .then((response) => {
      if (!checkStatus.checkRangeStatusCode(response.statusCode, lowerLimit, upperLimit)) {
        console.log(JSON.stringify(response.body, null, 2));
        // Set a bad Status error object
        return {
          StatusCode: response.statusCode,
          ErrorMessage: response.body,
          FuncName: requestType,
          InternalError: false
        };
      }
      // console.log(response.headers);
      // console.log(response.body);
      const resObj = {
        headers: response.headers,
        body: response.body,
        previousItem: item
      };
      // console.log(`******** ${requestType} *********`);
      // console.log(resObj);
      // console.log('----------------------------------');
      return resObj;
    })
    .catch((e) => {
      return {
        ErrorMessage: e,
        FuncName: requestType,
        InternalError: true
      };
    });
};

module.exports = expObj;

controller.js

//Tests response from API for different cards
Controller.testCard = (cardNum) => {
  const expMonth = new Date().getMonth() + 1;
  const expYear = new Date().getFullYear() + 2;
  const cardObj = {
    cardNum: cardNum,
    expMonth: expMonth,
    expYear: expYear
  };
  let apiCardItem = '';
  return testRequestToApi('getStripeToken', 200, 299, cardObj)
    .then((cardItem) => {
      return testRequestToApi('postNewCard', 200, 299, JSON.parse(cardItem.body));
    })
    .then((apiCard) => {
      apiCardItem = apiCard.body;
      try {
        apiCardItem = JSON.parse(apiCardItem);
      } catch(e) {
        console.log('Already a JSON object -----> Moving on');
      }
      return testRequestToApi('sampleAddToCart', 200, 299);
    })
    .then(() => testRequestToApi('useFailingStripeCards', 400, 499, apiCardItem.id))
    .then(() => testRequestToApi('deleteCard', 200, 299, apiCardItem.id));
};

//Loops through the card numbers and runs the test command against them
Controller.testAllCards = (cardsArray) => {
  return Promise.all(cardsArray.map((cardNum) => {
    return Controller.testCard(cardNum);
  }));
};

module.exports = Controller;

Good answer. I had virtually the same code ready, but got somewhat lost in details that are of more philosophical nature, so I figured pointing out the two main issues I saw would be enough for you to figure it out. :) — Tomalak, Jun 14 '17 at 14:31

score 0 · Answer 2 · answered Jun 14 '17 at 11:22

I rewrote your "testCard" function (controller.js)

//Tests response from API for different cards
const testCard = (cardNum) => {
  return new Promise((resolve, reject) => {
    let apiCardItem = '';
    const expDate = new Date();
    const cardObj = {
      cardNum:  cardNum,
      expMonth: expDate.getMonth() + 1,
      expYear:  expDate.getFullYear() + 2
    };
    
    testRequestToApi('getStripeToken', 200, 299, cardObj)

      .then((cardItem) => testRequestToApi('postNewCard', 200, 299, JSON.parse(cardItem.body)))

      .then((apiCard) => {
        apiCardItem = apiCard.body;
        try {
          apiCardItem = JSON.parse(apiCardItem);
        } catch(e) {
          console.log(e);
        }
        return testRequestToApi('sampleAddToCart', 200, 299);
      })

      .then(() => testRequestToApi('useFailingStripeCards', 400, 499, apiCardItem.id))

      .then(() => testRequestToApi('deleteCard', 200, 299, apiCardItem.id))

      .then(resolve)

      .catch(reject);
  });
};

And your "testRequestToApi" (test-request-to-api.js)

expObj.testRequestToApi = (requestType, lowerLimit, upperLimit, item) => {
  return new Promise((resolve, reject) => {
    const reqOps = formHeaders[requestType](item);

    request(reqOps, (err, response, body) => {
      let badStatus  = {};
      let badRequest = {};
      let resObj     = {};

      if (err) {
        badRequest = {
          ErrorMessage:  err,
          FuncName:      requestType,
          InternalError: true
        };

        reject(badRequest);
        return false;
      }
      
      if (!checkStatus.checkRangeStatusCode(response.statusCode, lowerLimit, upperLimit)) {
        console.log(JSON.stringify(body, null, 2));
        // Set a bad Status error object
        badStatus = {
          StatusCode:    response.statusCode,
          ErrorMessage:  body,
          FuncName:      requestType,
          InternalError: false
        };

        reject(badStatus);
        return false;
      }

      resObj = {
        headers: response.headers,
        body:    body
      };

      resolve(resObj);
    });
  });
};

I think the problem is when you return from a promise before a resolve/reject is getting called.

Also check that in nested promises you can pass resolve/reject to .then/catch for brevity.

Still doesn't work. Same issue. `'testAllCards has been run'` is still logged before the logs on other functions — hyprstack, Jun 14 '17 at 12:31

Nodejs promise all not running as expected

2 Answers2