Promise.all after a foreach loop

Question

I'm trying to use Promise.all() with an array of Promises that is being populated inside of a foreach looop right before but it seems like the Promise.all() is not waiting the promises to be all completed before executing its callback. What's wrong with the following code? (I tried to simplify it before posting, so parts of it might not make complete sense, but that promised and loops are all there).

class test {
  constructor(sql) {
    Promise.all([this.sync(sql, 0), this.sync(sql, 1)]).then((data) => {
      console.log(data);
    });
  }

  sync(sql, id = 0) {
    return new Promise((resolve, reject) => {
      request.get('http://localhost/test/' + id, {
        json: true
      }, (req, res) => {
        var promises = [];
        res.body['items'].forEach(item => {
          promises.push(new Promise((resolve, reject) => {
            this.existingRecord(sql, item['id']).then(() => {
              resolve(false);
            }).catch(() => {
              this.add(sql, item).then(resolve(id));
            })
          }))
        });
        Promise.all(promises).then((data) => resolve(data));
      });
    });
  }

  add(sql, data) {
    return new Promise((resolve, reject) => {
      console.log('Inserting ' + data['id']);
      var request = new sql.Request();
      var query = `INSERT INTO test (col1, col2) VALUES (${utils.prepareInsertdata(data)})`;
      request.query(query, (err, result) => {
        if (err) {
          console.log('ERROR INSERTING: ' + data['id']);
          console.log(err);
        }
        resolve();
      });
    });
  }
}

how did you check if it's not waiting for all promises to completed? — Amit Chauhan, Jul 05 '19 at 04:25
Save yourself = https://github.com/request/request-promise-native — Randy Casburn, Jul 05 '19 at 04:44
Hi @eramit2010, I know it's not waiting because console.log('Inserting ' + data['id']); is not outputted to the console. — Leandro Lorenzini de Oliveira, Jul 05 '19 at 05:11
did you try checking if you are actually getting data correct? putting console inside your promise which you are pushing into promises? — Amit Chauhan, Jul 05 '19 at 05:14
@eramit2010, yes. Actually before I was not using promises and the program was running fine, I decided to add those promises so that I know once the execution has finished. — Leandro Lorenzini de Oliveira, Jul 05 '19 at 05:17
no i meant you have added console in add function, did you try checking what is happening in promises which are there in forEach promises? — Amit Chauhan, Jul 05 '19 at 05:24
If I see correctly, `add` is only fired when `this.existingRecord` throws. How does `console.log` in `add` not printing imply promises are not waiting? — Amadan, Jul 05 '19 at 05:45
I think `Promise.all(promises).then((data) => resolve(data));` should be written inside the `resolve()` because you are using resolve() for the inner promise but there isn't any resolve() for outer promise. — Rupesh, Jul 05 '19 at 06:01

score 1 · Accepted Answer · answered Jul 05 '19 at 07:53

First off, you're making it much harder to write good, clean, error-free code when you have a mix of promises and regular callbacks in your control flow. I find that the best way to write asynchronous code using promises is to first take any asynchronous operations that are not promise based and create promise-based wrappers for them and then write my logic and control flow using only promises. This makes a consistent path for flow of control and for error handling and it removes the mess of promisifying things from the actual main logic.

Then, I see several significant issues in your code.

Asynchronous operations in the constructor

It's almost never a good idea to put asynchronous operations in the constructor. This is because the constructor HAS to return the object itself so that leaves no simple way to communicate back to the code that created your object when the asynchronous operations are actually done and if they succeeded of failed. It is not entirely clear to me what you're trying to accomplish with those async operations, but this is likely a bad design pattern. I favor a factory function that returns a promise that resolves to the new object for combining the creation of an object with asynchronous operations. This gives you everything you need, a fully formed object, knowledge of when the async operations are done and an ability to have error handling for the async operations. You can see more about this factory function option and some other design options here:

Asynchronous operations in constructor

Improve .then() handler construction

When you do this:

this.add(sql, item).then(resolve(id));

You are calling resolve(id) immediately and passing that to .then() rather than waiting for the .then() handler to be called before calling resolve(id). All of this is complicated because you're mixing regular callbacks and promises.

Creating new wrapped promises rather than just returning existing promises

This is related to your mix of regular callbacks and regular promises, but you'd much rather just return an existing promise than wrap it in a new promise that you have to manually resolve and reject. More than half the time, you will miss proper error handling when manually wrapping things in a new promise and it just results in more code than is needed.

Race Conditions

In any sort of multi-user database environment, you can't write database code such as:

if (record exists) {
    do one thing
} else {
    create new record
}

This is a race condition. If some other database request comes in during the processing of this, it could change the database in the middle of this and you'd be trying to create a record that just got created by another piece of code.

The usual solution varies by database (and you don't say exactly which database library you're using). Usually, you want to let the database manage the creation of unique records making it so that a duplicate record (by whatever key you're managing uniqueness in this table by) isn't allowed in the database and the concurrency of that is managed by the database itself. Some databases have an atomic operation such as findOrCreate() that will find an existing record or create a new one in an atomic fashion. Other databases have other approaches. But, it's important to make sure that adding unique records to the database is an atomic operation that can't ever create unwanted duplicates.

I'd suggest this implementation:

// use promise version of request library (already promisified for us)
const rp = require('request-promise');

class test {
    constructor() {

    }

    init(sql) {
        return Promise.all([this.sync(sql, 0), this.sync(sql, 1)]).then((data) => {
            console.log(data);
            // do something with the data here - probably store it in instance data
        });

    }

    sync(sql, id = 0) {
        return rp.get('http://localhost/test/' + id, {json: true}).then(res => {
            // process all items
            return Promise.all(res.body.items.map(item => {
                return this.existingRecord(sql, item.id).then(() => {
                    return false;
                }).catch(() => {
                    // it's probably bad form here to do this for all possible database errors
                    // probably this should be looking for a specific error of id not found
                    // or something like that.
                    // This is also likely a race condition.  You would typically avoid the race
                    // condition by making the item key unique in the database and just doing an add and letting
                    // the database tell you the add failed because the item already exists
                    // This will allow the database to control the concurrency and avoid race conditions
                    return this.add(sql, item);
                });
            }));
        });
    }

}


// factory function that returns promise that resolves to a new object
// don't use new test() elsewhere
function createTestObj(sql) {
    let t = new test();
    return t.init(sql).then(() => {
        // resolve to our new object
        return t;
    });
}

For your add() method, I'd switch to using the promise interface in your sql database. There should either be one built-in or a 3rd party package that will add one on top of your database interface. This will prevent the manual creation of promises and the incomplete error handling in your add() method.

Promise.all after a foreach loop

1 Answers1