Use Promise.all With Caution

19 Feb, 2024

A few weeks ago, I found two bugs in two different projects whose root causes were both careless use of Promise.all.

First, let's briefly review how promises behave when Promise.all is not used. For our scenario, let's imagine we need to make multiple network calls.

fetch('http://example.com/1')
  .then(() => fetch('http://example.com/2'))
  .then(() => fetch('http://example.com/3'));

// Or using async/await...

async function example() {
  await fetch('http://example.com/1');
  await fetch('http://example.com/2');
  await fetch('http://example.com/3');
}

In this example, a network call is made to http://example.com/1. The code waits until that call is complete. Then it makes a call to http://example.com/2 and waits for that call to complete. Finally, a call is made to http://example.com/3. It's important to note that only one network call is made at a time.

Don't get too caught up on the fact that we're making network calls. I used this as an example because most JavaScript developers will be familiar with fetch as a function that returns a promise. You can substitute any type of promise.

These promises, while asynchronous, are being run serially. Promise.all can be a performance super power that may allow multiple promises to run in parallel.¹ It finally returns when either all of the promises passed to it resolve, or when any one promise is rejected.

Promise.all([
  fetch('http://example.com/1'),
  fetch('http://example.com/2'),
  fetch('http://example.com/3'),
]);

In the typical case, this version should run faster than the serial example because all three promises can run simultaneously.

However, it's very important to think critically before applying Promise.all. Here's a simplified version of the first bug I encountered.

const items = await Item.findAll({
  where: { user_id: id, checked: false },
});

Promise.all(
  items.map(item => item.update({ checked: true })
);

At first glance, this seems like a reasonable thing to do. Updating all the items simultaneously will speed this up, right? In actuality, this resulted in an error.

The first warning sign is that there is no limit on the number of items that belong to this particular user. Most of the time this will probably be fine. But what if a user has an idiosyncratic number of unchecked items? What if it's hundreds or thousands? Your server is going to attempt to run an unhealthy number of promises at once.²

In the real code, this was using the ORM Sequelize. Sequelize keeps a pool of database connections. Whenever you make a call to the database, a connection from the pool is used. Most of the time, only one connection is used per request. If you're doing something fancy with Promise.all, maybe more than one connection is used simultaneously per request. In this case, if the number of items exceeds the maximum number of connections allowed in the pool, you'll receive an error, which is exactly what happened.

To fix this, you could increase the maximum number of connections allowed in the pool. But how many is enough? Because items are created by users, there is no realistic maximum number. I chose to make the promises run serially instead.

I had run into similar issues regarding simultaneous database calls in the past and I had already added the Bluebird library to this particular project. It includes a function called mapSeries which does exactly what I need.

import * as P from 'bluebird';

// ...snip...

P.mapSeries(items, item => item.update({ checked: true });

If you don't like Bluebird, there are other libraries available to control the level of concurrency when Promise.all is used. Some of them even include the ability to specify concurrency limits higher than 1. I encourage the reader to experiment and see if they can roll their own function that takes an array of promises and awaits them serially.

The other bug I encountered was in a different project, but was once again a misapplication of Promise.all. In this case, it was in a database migration for use with the Knex ORM.

export function up(knex) {
  return Promise.all([
    knex.schema.table('table_a', (table) => {
      table.index('column_a');
    }),
    knex.schema.table('table_b', (table) => {
      table.index('column_a');
    }),
  ]);
};

I didn't dive too deeply into the root cause of this migration failing because I'd already been primed to be suspicious of unnecessary use of Promise.all by the previous bug from earlier in the week. My best guess is that because table_b has a foreign key that points to table_a, the database threw an error when this migration attempted to alter table_b while table_a was still in the process of being altered.

In any case, the solution was very straightforward. Use Knex's promise chaining as it was designed to be used.

export function up(knex) {
  return knex.schema
    .table('table_a', (table) => {
      table.index('column_a');
    })
    .table('table_b', (table) => {
      table.index('column_a');
    });
};

This version of the migration worked without issue.

These examples show the need to think critically whenever you reach for Promise.all or any other promise utility that results in parallelism in your code.

• Don't use Promise.all if you don't know how many promises you may be passing in ahead of time.
• Don't use Promise.all if your promises all depend on a shared, limited resource.
• Don't use Promise.all if the order in which the promises resolve matters.

#javascript #typescript