From 9c6557e8ec9d55d973f19444bf189fb54a5e78c2 Mon Sep 17 00:00:00 2001 From: Drew Anderson Date: Wed, 22 Dec 2021 15:47:47 -0600 Subject: [PATCH 1/2] Change the way the MultiItemCache iterates over items to prevent stack exhaustion when it has a large number of items. Added unit tests for MultiItemCache behavior, including one for covering the large item count case. --- lib/CacheFacade.js | 22 +++++++---- test/MultiItemCache.unittest.js | 65 +++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 7 deletions(-) create mode 100644 test/MultiItemCache.unittest.js diff --git a/lib/CacheFacade.js b/lib/CacheFacade.js index acd22c7405e..aacba78b9ce 100644 --- a/lib/CacheFacade.js +++ b/lib/CacheFacade.js @@ -46,15 +46,23 @@ class MultiItemCache { * @returns {void} */ get(callback) { - const next = i => { - this._items[i].get((err, result) => { + let i = 0; + let resultFound = false; + const generateGetCallback = (/** @type {Boolean} */ lastItem) => { + return ( + /** @type {import("./WebpackError")} */ err, + /** @type {T} */ res + ) => { + if (resultFound) return; + resultFound = !!(err || res || false); if (err) return callback(err); - if (result !== undefined) return callback(null, result); - if (++i >= this._items.length) return callback(); - next(i); - }); + if (res !== undefined) return callback(null, res); + if (lastItem) return callback(); + }; }; - next(0); + do { + this._items[i].get(generateGetCallback(i === this._items.length - 1)); + } while (++i < this._items.length); } /** diff --git a/test/MultiItemCache.unittest.js b/test/MultiItemCache.unittest.js new file mode 100644 index 00000000000..7fddfd17714 --- /dev/null +++ b/test/MultiItemCache.unittest.js @@ -0,0 +1,65 @@ +"use strict"; + +const Cache = require("../lib/Cache"); +const { ItemCacheFacade, MultiItemCache } = require("../lib/CacheFacade"); + +describe("MultiItemCache", () => { + it("Throws when getting items from an empty Cache", () => { + const multiItemCache = new MultiItemCache(generateItemCaches(0)); + expect(() => multiItemCache.get(_ => _())).toThrowError(); + }); + + it("Returns the single ItemCacheFacade when passed an array of length 1", () => { + const itemCaches = generateItemCaches(1); + const multiItemCache = new MultiItemCache(itemCaches); + expect(multiItemCache).toBe(itemCaches[0]); + }); + + it("Retrieves items from the underlying Cache when get is called", () => { + const itemCaches = generateItemCaches(10); + const multiItemCache = new MultiItemCache(itemCaches); + const callback = (err, res) => { + expect(err).toBeNull(); + expect(res).toBeInstanceOf(Object); + }; + for (let i = 0; i < 10; ++i) { + multiItemCache.get(callback); + } + }); + + it("Can get() a large number of items without exhausting the stack", () => { + const itemCaches = generateItemCaches(10000, () => undefined); + const multiItemCache = new MultiItemCache(itemCaches); + let callbacks = 0; + const callback = (err, res) => { + expect(err).toBeUndefined(); + expect(res).toBeUndefined(); + ++callbacks; + }; + multiItemCache.get(callback); + expect(callbacks).toEqual(1); + }); + + function generateItemCaches(howMany, dataGenerator) { + const ret = []; + for (let i = 0; i < howMany; ++i) { + const name = `ItemCache${i}`; + const tag = `ItemTag${i}`; + const dataGen = + dataGenerator || + (() => { + return { name: tag }; + }); + const cache = new Cache(); + cache.hooks.get.tapAsync( + "DataReturner", + (_identifier, _etag, _gotHandlers, callback) => { + callback(undefined, dataGen()); + } + ); + const itemCache = new ItemCacheFacade(cache, name, tag); + ret[i] = itemCache; + } + return ret; + } +}); From b54f2ac65c37090d826d462c7fc3399bda4e1ccc Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Mon, 10 Jan 2022 14:00:18 +0100 Subject: [PATCH 2/2] use forEachBail from enhanced-resolve --- lib/CacheFacade.js | 19 ++----------------- test/MultiItemCache.unittest.js | 2 +- 2 files changed, 3 insertions(+), 18 deletions(-) diff --git a/lib/CacheFacade.js b/lib/CacheFacade.js index aacba78b9ce..89c7658b580 100644 --- a/lib/CacheFacade.js +++ b/lib/CacheFacade.js @@ -5,6 +5,7 @@ "use strict"; +const { forEachBail } = require("enhanced-resolve"); const asyncLib = require("neo-async"); const getLazyHashedEtag = require("./cache/getLazyHashedEtag"); const mergeEtags = require("./cache/mergeEtags"); @@ -46,23 +47,7 @@ class MultiItemCache { * @returns {void} */ get(callback) { - let i = 0; - let resultFound = false; - const generateGetCallback = (/** @type {Boolean} */ lastItem) => { - return ( - /** @type {import("./WebpackError")} */ err, - /** @type {T} */ res - ) => { - if (resultFound) return; - resultFound = !!(err || res || false); - if (err) return callback(err); - if (res !== undefined) return callback(null, res); - if (lastItem) return callback(); - }; - }; - do { - this._items[i].get(generateGetCallback(i === this._items.length - 1)); - } while (++i < this._items.length); + forEachBail(this._items, (item, callback) => item.get(callback), callback); } /** diff --git a/test/MultiItemCache.unittest.js b/test/MultiItemCache.unittest.js index 7fddfd17714..5fde32e7d1f 100644 --- a/test/MultiItemCache.unittest.js +++ b/test/MultiItemCache.unittest.js @@ -32,7 +32,7 @@ describe("MultiItemCache", () => { const multiItemCache = new MultiItemCache(itemCaches); let callbacks = 0; const callback = (err, res) => { - expect(err).toBeUndefined(); + expect(err).toBeNull(); expect(res).toBeUndefined(); ++callbacks; };