From 346111250e87fa82b3f8e955a6fb24cd219870d0 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Wed, 16 Sep 2020 16:59:00 -0400
Subject: [PATCH] utils: Fix promise creation accounting bug in
 promises.timesLimit

Before this change, `promises.timesLimit()` created `concurrency - 1`
too many promises. The only users of this function use a concurrency
of 500, so this meant that 499 extra promises were created each time
it was used. The bug didn't affect correctness, but it did result in a
large number of unnecessary database operations whenever a pad was
deleted. This change fixes that bug.

Also:
  * Convert the function to async and have it resolve after all of the
    created promises are resolved.
  * Reject concurrency of 0 (unless total is 0).
  * Document the function.
  * Add tests.
---
 src/node/utils/promises.js      | 38 +++++++--------
 tests/backend/specs/promises.js | 85 +++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+), 22 deletions(-)
 create mode 100644 tests/backend/specs/promises.js

diff --git a/src/node/utils/promises.js b/src/node/utils/promises.js
index a754823a0..bb973befa 100644
--- a/src/node/utils/promises.js
+++ b/src/node/utils/promises.js
@@ -35,27 +35,21 @@ exports.firstSatisfies = (promises, predicate) => {
   return Promise.race(newPromises);
 };
 
-exports.timesLimit = function(ltMax, concurrency, promiseCreator) {
-  var done = 0
-  var current = 0
-
-  function addAnother () {
-    function _internalRun () {
-      done++
-
-      if (done < ltMax) {
-        addAnother()
-      }
-    }
-
-    promiseCreator(current)
-      .then(_internalRun)
-      .catch(_internalRun)
-
-    current++
-  }
-
-  for (var i = 0; i < concurrency && i < ltMax; i++) {
-    addAnother()
+// Calls `promiseCreator(i)` a total number of `total` times, where `i` is 0 through `total - 1` (in
+// order). The `concurrency` argument specifies the maximum number of Promises returned by
+// `promiseCreator` that are allowed to be active (unresolved) simultaneously. (In other words: If
+// `total` is greater than `concurrency`, then `concurrency` Promises will be created right away,
+// and each remaining Promise will be created once one of the earlier Promises resolves.) This async
+// function resolves once all `total` Promises have resolved.
+exports.timesLimit = async (total, concurrency, promiseCreator) => {
+  if (total > 0 && concurrency <= 0) throw new RangeError('concurrency must be positive');
+  let next = 0;
+  const addAnother = () => promiseCreator(next++).finally(() => {
+    if (next < total) return addAnother();
+  });
+  const promises = [];
+  for (var i = 0; i < concurrency && i < total; i++) {
+    promises.push(addAnother());
   }
+  await Promise.all(promises);
 }
diff --git a/tests/backend/specs/promises.js b/tests/backend/specs/promises.js
new file mode 100644
index 000000000..13a8c532a
--- /dev/null
+++ b/tests/backend/specs/promises.js
@@ -0,0 +1,85 @@
+function m(mod) { return __dirname + '/../../../src/' + mod; }
+
+const assert = require('assert').strict;
+const promises = require(m('node/utils/promises'));
+
+describe('promises.timesLimit', async () => {
+  let wantIndex = 0;
+  const testPromises = [];
+  const makePromise = (index) => {
+    // Make sure index increases by one each time.
+    assert.equal(index, wantIndex++);
+    // Save the resolve callback (so the test can trigger resolution)
+    // and the promise itself (to wait for resolve to take effect).
+    const p = {};
+    const promise = new Promise((resolve) => {
+      p.resolve = resolve;
+    });
+    p.promise = promise;
+    testPromises.push(p);
+    return p.promise;
+  };
+
+  const total = 11;
+  const concurrency = 7;
+  const timesLimitPromise = promises.timesLimit(total, concurrency, makePromise);
+
+  it('honors concurrency', async () => {
+    assert.equal(wantIndex, concurrency);
+  });
+
+  it('creates another when one completes', async () => {
+    const {promise, resolve} = testPromises.shift();
+    resolve();
+    await promise;
+    assert.equal(wantIndex, concurrency + 1);
+  });
+
+  it('creates the expected total number of promises', async () => {
+    while (testPromises.length > 0) {
+      // Resolve them in random order to ensure that the resolution order doesn't matter.
+      const i = Math.floor(Math.random() * Math.floor(testPromises.length));
+      const {promise, resolve} = testPromises.splice(i, 1)[0];
+      resolve();
+      await promise;
+    }
+    assert.equal(wantIndex, total);
+  });
+
+  it('resolves', async () => {
+    await timesLimitPromise;
+  });
+
+  it('does not create too many promises if total < concurrency', async () => {
+    wantIndex = 0;
+    assert.equal(testPromises.length, 0);
+    const total = 7;
+    const concurrency = 11;
+    const timesLimitPromise = promises.timesLimit(total, concurrency, makePromise);
+    while (testPromises.length > 0) {
+      const {promise, resolve} = testPromises.pop();
+      resolve();
+      await promise;
+    }
+    await timesLimitPromise;
+    assert.equal(wantIndex, total);
+  });
+
+  it('accepts total === 0, concurrency > 0', async () => {
+    wantIndex = 0;
+    assert.equal(testPromises.length, 0);
+    await promises.timesLimit(0, concurrency, makePromise);
+    assert.equal(wantIndex, 0);
+  });
+
+  it('accepts total === 0, concurrency === 0', async () => {
+    wantIndex = 0;
+    assert.equal(testPromises.length, 0);
+    await promises.timesLimit(0, 0, makePromise);
+    assert.equal(wantIndex, 0);
+  });
+
+  it('rejects total > 0, concurrency === 0', async () => {
+    await assert.rejects(promises.timesLimit(total, 0, makePromise), RangeError);
+  });
+});