Nodejs express promise.all().then() is not synchronous

Question

I'm building a little app using Nodejs express where I update/delete some data. After it did the update/delete, I want to return the data from the database again. I'm using promise.all() because I want to either have everything succeed or fail together.

My problem is that it's not being run synchronously. Every now and then my code returns the data before the update/delete has happend.

Here is an example:

const save = async (houseId, personIds, pets, petsToDelete) => {
    let dbActions = []

    personIds.forEach((personId) => {
        pets.forEach((pet) => {
            pet.ownerId = personId
        })
        dbActions.push(
            PetRepository.deletePets(personId, petsToDelete),
            PetRepository.updatePets(personId, pets),
        )
    })

    await Promise.all(dbActions)
    let promiseResult = await Promise.all([
           PetRepository.findByHouse({ houseId: houseId })
    ])

    return promiseResult
}

The issue is that PetRepository.findByHouse is sometimes returning data before all dbActions in the loop were executed/completed.

I thought adding them to an array and using that array in a Promise.all outside the loop would fix the issue, but it didn't. Any ideas?

How save() is called:

    const promiseResponseHandler = (req, res, promise) => {
        promise.then(successResponse(req, res)).catch(...errorhandling...)
    }

    App.put('save/:houseId', (req, res) => {
        promiseResponseHandler(req, res, PetService.save(req.params.houseId, req.body[0], req.body[1], req.body[2]))
    })

Update: repository functions

const Mongoose = require('mongoose')
Mongoose.Promise = global.Promise

Mongoose.connection.on('open', () => Logger.info('Db connection established'))
let connectionString

connectionString = `mongodb+srv://${Config.MONGO_HOST}?retryWrites=true`

connection = Mongoose.connect(
    connectionString,
    {
        user: Config.MONGO_USER,
        pass: Config.MONGO_PW,
        dbName: Config.MONGO_DB,
        reconnectTries: 3,
        useNewUrlParser: true
    }
).catch((err) => {
    Logger.error(err)
})

const deletePets = async (personId, petsToDelete) => {
    if (petsToDelete.length > 0) {
        await Pet.bulkWrite(
            petsToDelete.map((petId) => {
                deleteMany: {
                    filter: { personId: personId, petId: petId}
                }
            })
        )
    }
}

const updatePets = async (personId, pets) => {
    if (pets.length > 0) {
        await Pet.bulkWrite(
            pets.map((pet) => ({
                updateOne: {
                    filter: { personId: pet.personId, petId: petId },
                    update: { 
                        $set: { name: pet.name },
                        $setOnInsert: { 
                            petId: pet.petId, 
                            personId: personId,
                            houseId: pet.houseId
                        } 
                    }
                }
            }))
        )
    }
}

module.exports = { deletePets, updatePets }

Answer 1

You are using thenables (callbacks) to approach this problem. It would be easier to use async/await. In order to be able to await the Promise.all, you would need to define save as an asynchronous function, like so:

const save = async (houseId, personIds, pets, petsToDelete) => {
    let dbActions = []

    personIds.forEach((personId) => {
        pets.forEach((pet) => {
            pet.ownerId = personId
        })
        dbActions.push(
            PetRepository.deletePets(personId, petsToDelete),
            PetRepository.updatePets(personId, pets),
        )
    })

    await Promise.all(dbActions)
    let promiseResult = await PetRepository.findByHouse({ houseId: houseId })
    return promiseResult
}

You would then proceed to await the save in order to make sure it concludes before you continue. Of course, you could do this with callbacks too, passing it to the save method, and calling it with your final response, like so:

const save = (houseId, personIds, pets, petsToDelete, callback) => {
    let dbActions = []

    personIds.forEach((personId) => {
        pets.forEach((pet) => {
            pet.ownerId = personId
        })
        dbActions.push(
            PetRepository.deletePets(personId, petsToDelete),
            PetRepository.updatePets(personId, pets),
        )
    })

    Promise.all(dbActions).then(()=>
        // callback gets called with the response of the promise
        PetRepository.findByHouse({ houseId: houseId }).then(callback)
    )
}

You do have to keep in mind that you can't just assign a promise to a variable and expect it to be the result. If you do:

let result = Promise.all([promise1, promise2])

result will contain a promise, so in order to get the value you'll either have to await it (inline) or then it (callback)

Answer 2

I believe I have found what has been causing PetRepository.findByHouse to return prematurely.

const save = (houseId, personIds, pets, petsToDelete) => {
    let dbActions = []

    personIds.forEach((personId) => {
        pets.forEach((pet) => {
            pet.ownerId = personId
        })
        dbActions.push(
            PetRepository.deletePets(personId, petsToDelete),
            PetRepository.updatePets(personId, pets),
        )
    })

    return Promise.all(dbActions).then(() => { // this is the correct way to wait for dbActions
                                               // and then return findByHouse(...)
            return PetRepository.findByHouse({ houseId: houseId })
    })
}

Answer 3

function then expects function to be provided but you provide just assignment and this assignment executes in a same time. So instead of

Promise.all(dbActions).then( 
        (promiseResult = Promise.all([
            PetRepository.findByHouse({ houseId: houseId })
        ]))
    )

write at least

Promise.all(dbActions).then( () =>
        (promiseResult = Promise.all([
            PetRepository.findByHouse({ houseId: houseId })
        ]))
    )

but even this code looks strange. Why you put one promise inside Promise.all, and what do you expect to get in promiseResult - promise or result?

May be you meant

Promise.all(dbActions).then( () =>
  PetRepository.findByHouse({ houseId: houseId })
).then(res => { promiseResult = res })

then in promiseResult you will get Pet instance itself