From 2db6c085ea08ee639767d37e6fd83a1ca0fbd9ce Mon Sep 17 00:00:00 2001 From: Gar Date: Wed, 24 Aug 2022 12:55:44 -0700 Subject: [PATCH] fix: loadActual cleanup While looking at the target setter for nodes, it seemed odd that we were making affordances for sometimes setting a promise as the target. After unraveling the code, it turns out this is impossible outside of tests, where we set environment variables to mimic that state. We were always awaiting the generation of links/nodes where appropriate. This commit inlines some code and cleans it up to the point where this fact could be verified, and then removes the now unneeded logic in loadActual that was trying to account for this. loadActual, an async function, now returns a promise that resolves to the tree, as a singleton. This maintains the use case commented on where buildIdealTree and reify can happen in parallel. This fixes a potential bug in reify (and likely others) which pass around this.idealTree as an object, and NOT a promise, even though before this refactor it can sometimes be a promise. --- .../arborist/lib/arborist/load-actual.js | 429 ++++++++---------- 1 file changed, 196 insertions(+), 233 deletions(-) diff --git a/workspaces/arborist/lib/arborist/load-actual.js b/workspaces/arborist/lib/arborist/load-actual.js index 7ab65f5b00d8b..bb813806e5556 100644 --- a/workspaces/arborist/lib/arborist/load-actual.js +++ b/workspaces/arborist/lib/arborist/load-actual.js @@ -15,34 +15,30 @@ const Node = require('../node.js') const Link = require('../link.js') const realpath = require('../realpath.js') -const _loadFSNode = Symbol('loadFSNode') -const _newNode = Symbol('newNode') -const _newLink = Symbol('newLink') -const _loadFSTree = Symbol('loadFSTree') -const _loadFSChildren = Symbol('loadFSChildren') -const _findMissingEdges = Symbol('findMissingEdges') -const _findFSParents = Symbol('findFSParents') -const _resetDepFlags = Symbol('resetDepFlags') - -const _actualTreeLoaded = Symbol('actualTreeLoaded') +// public symbols +const _changePath = Symbol.for('_changePath') +const _global = Symbol.for('global') +const _loadWorkspaces = Symbol.for('loadWorkspaces') const _rpcache = Symbol.for('realpathCache') const _stcache = Symbol.for('statCache') -const _topNodes = Symbol('linkTargets') + +// private symbols +const _actualTree = Symbol('actualTree') +const _actualTreeLoaded = Symbol('actualTreeLoaded') +const _actualTreePromise = Symbol('actualTreePromise') const _cache = Symbol('nodeLoadingCache') +const _filter = Symbol('filter') +const _findMissingEdges = Symbol('findMissingEdges') const _loadActual = Symbol('loadActual') -const _loadActualVirtually = Symbol('loadActualVirtually') -const _loadActualActually = Symbol('loadActualActually') -const _loadWorkspaces = Symbol.for('loadWorkspaces') -const _loadWorkspaceTargets = Symbol('loadWorkspaceTargets') -const _actualTreePromise = Symbol('actualTreePromise') -const _actualTree = Symbol('actualTree') +const _loadFSChildren = Symbol('loadFSChildren') +const _loadFSNode = Symbol('loadFSNode') +const _loadFSTree = Symbol('loadFSTree') +const _newLink = Symbol('newLink') +const _newNode = Symbol('newNode') +const _topNodes = Symbol('linkTargets') const _transplant = Symbol('transplant') const _transplantFilter = Symbol('transplantFilter') -const _filter = Symbol('filter') -const _global = Symbol.for('global') -const _changePath = Symbol.for('_changePath') - module.exports = cls => class ActualLoader extends cls { constructor (options) { super(options) @@ -75,37 +71,44 @@ module.exports = cls => class ActualLoader extends cls { this[_topNodes] = new Set() } - [_resetDepFlags] (tree, root) { - // reset all deps to extraneous prior to recalc - if (!root) { - for (const node of tree.inventory.values()) { - node.extraneous = true - } - } - - // only reset root flags if we're not re-rooting, - // otherwise leave as-is - calcDepFlags(tree, !root) - return tree - } - // public method async loadActual (options = {}) { - // allow the user to set options on the ctor as well. - // XXX: deprecate separate method options objects. - options = { ...this.options, ...options } - - // stash the promise so that we don't ever have more than one - // going at the same time. This is so that buildIdealTree can - // default to the actualTree if no shrinkwrap present, but - // reify() can still call buildIdealTree and loadActual in parallel - // safely. - return this.actualTree ? this.actualTree - : this[_actualTreePromise] ? this[_actualTreePromise] - : this[_actualTreePromise] = this[_loadActual](options) - .then(tree => this[_resetDepFlags](tree, options.root)) - .then(tree => this.actualTree = treeCheck(tree)) + // In the past this.actualTree was set as a promise that eventually + // resolved, and overwrite this.actualTree with the resolved value. This + // was a problem because virtually no other code expects this.actualTree to + // be a promise. Instead we only set it once resolved, and also return it + // from the promise so that it is what's returned from this function when + // awaited. + if (this.actualTree) { + return this.actualTree + } + if (!this[_actualTreePromise]) { + // allow the user to set options on the ctor as well. + // XXX: deprecate separate method options objects. + options = { ...this.options, ...options } + + this[_actualTreePromise] = this[_loadActual](options) + .then(tree => { + // reset all deps to extraneous prior to recalc + if (!options.root) { + for (const node of tree.inventory.values()) { + node.extraneous = true + } + } + + // only reset root flags if we're not re-rooting, + // otherwise leave as-is + calcDepFlags(tree, !options.root) + this.actualTree = treeCheck(tree) + return this.actualTree + }) + } + return this[_actualTreePromise] } + // return the promise so that we don't ever have more than one going at the + // same time. This is so that buildIdealTree can default to the actualTree + // if no shrinkwrap present, but reify() can still call buildIdealTree and + // loadActual in parallel safely. async [_loadActual] (options) { // mostly realpath to throw if the root doesn't exist @@ -122,75 +125,102 @@ module.exports = cls => class ActualLoader extends cls { if (global) { const real = await realpath(this.path, this[_rpcache], this[_stcache]) - const newNodeOrLink = this.path === real ? _newNode : _newLink - this[_actualTree] = await this[newNodeOrLink]({ + const params = { path: this.path, realpath: real, pkg: {}, global, loadOverrides: true, + } + if (this.path === real) { + this[_actualTree] = this[_newNode](params) + } else { + this[_actualTree] = await this[_newLink](params) + } + } else { + // not in global mode, hidden lockfile is allowed, load root pkg too + this[_actualTree] = await this[_loadFSNode]({ + path: this.path, + real: await realpath(this.path, this[_rpcache], this[_stcache]), + loadOverrides: true, }) - return this[_loadActualActually]({ root, ignoreMissing, global }) - } - // not in global mode, hidden lockfile is allowed, load root pkg too - this[_actualTree] = await this[_loadFSNode]({ - path: this.path, - real: await realpath(this.path, this[_rpcache], this[_stcache]), - loadOverrides: true, - }) + this[_actualTree].assertRootOverrides() + + // if forceActual is set, don't even try the hidden lockfile + if (!forceActual) { + // Note: hidden lockfile will be rejected if it's not the latest thing + // in the folder, or if any of the entries in the hidden lockfile are + // missing. + const meta = await Shrinkwrap.load({ + path: this[_actualTree].path, + hiddenLockfile: true, + resolveOptions: this.options, + }) + + if (meta.loadedFromDisk) { + this[_actualTree].meta = meta + // have to load on a new Arborist object, so we don't assign + // the virtualTree on this one! Also, the weird reference is because + // we can't easily get a ref to Arborist in this module, without + // creating a circular reference, since this class is a mixin used + // to build up the Arborist class itself. + await new this.constructor({ ...this.options }).loadVirtual({ + root: this[_actualTree], + }) + await this[_loadWorkspaces](this[_actualTree]) - this[_actualTree].assertRootOverrides() + this[_transplant](root) + return this[_actualTree] + } + } - // if forceActual is set, don't even try the hidden lockfile - if (!forceActual) { - // Note: hidden lockfile will be rejected if it's not the latest thing - // in the folder, or if any of the entries in the hidden lockfile are - // missing. const meta = await Shrinkwrap.load({ path: this[_actualTree].path, - hiddenLockfile: true, + lockfileVersion: this.options.lockfileVersion, resolveOptions: this.options, }) - - if (meta.loadedFromDisk) { - this[_actualTree].meta = meta - return this[_loadActualVirtually]({ root }) - } + this[_actualTree].meta = meta } - const meta = await Shrinkwrap.load({ - path: this[_actualTree].path, - lockfileVersion: this.options.lockfileVersion, - resolveOptions: this.options, - }) - this[_actualTree].meta = meta - return this[_loadActualActually]({ root, ignoreMissing }) - } - - async [_loadActualVirtually] ({ root }) { - // have to load on a new Arborist object, so we don't assign - // the virtualTree on this one! Also, the weird reference is because - // we can't easily get a ref to Arborist in this module, without - // creating a circular reference, since this class is a mixin used - // to build up the Arborist class itself. - await new this.constructor({ ...this.options }).loadVirtual({ - root: this[_actualTree], - }) + await this[_loadFSTree](this[_actualTree]) await this[_loadWorkspaces](this[_actualTree]) - this[_transplant](root) - return this[_actualTree] - } + // if there are workspace targets without Link nodes created, load + // the targets, so that we know what they are. + if (this[_actualTree].workspaces && this[_actualTree].workspaces.size) { + const promises = [] + for (const path of this[_actualTree].workspaces.values()) { + if (!this[_cache].has(path)) { + // workspace overrides use the root overrides + const p = this[_loadFSNode]({ path, root: this[_actualTree], useRootOverrides: true }) + .then(node => this[_loadFSTree](node)) + promises.push(p) + } + } + await Promise.all(promises) + } - async [_loadActualActually] ({ root, ignoreMissing, global }) { - await this[_loadFSTree](this[_actualTree]) - await this[_loadWorkspaces](this[_actualTree]) - await this[_loadWorkspaceTargets](this[_actualTree]) if (!ignoreMissing) { await this[_findMissingEdges]() } - this[_findFSParents]() + + // try to find a node that is the parent in a fs tree sense, but not a + // node_modules tree sense, of any link targets. this allows us to + // resolve deps that node will find, but a legacy npm view of the + // world would not have noticed. + for (const path of this[_topNodes]) { + const node = this[_cache].get(path) + if (node && !node.parent && !node.fsParent) { + for (const p of walkUp(dirname(path))) { + if (this[_cache].has(p)) { + node.fsParent = this[_cache].get(p) + break + } + } + } + } + this[_transplant](root) if (global) { @@ -209,25 +239,6 @@ module.exports = cls => class ActualLoader extends cls { return this[_actualTree] } - // if there are workspace targets without Link nodes created, load - // the targets, so that we know what they are. - async [_loadWorkspaceTargets] (tree) { - if (!tree.workspaces || !tree.workspaces.size) { - return - } - - const promises = [] - for (const path of tree.workspaces.values()) { - if (!this[_cache].has(path)) { - // workspace overrides use the root overrides - const p = this[_loadFSNode]({ path, root: this[_actualTree], useRootOverrides: true }) - .then(node => this[_loadFSTree](node)) - promises.push(p) - } - } - await Promise.all(promises) - } - [_transplant] (root) { if (!root || root === this[_actualTree]) { return @@ -248,80 +259,63 @@ module.exports = cls => class ActualLoader extends cls { this[_actualTree] = root } - [_loadFSNode] ({ path, parent, real, root, loadOverrides, useRootOverrides }) { + async [_loadFSNode] ({ path, parent, real, root, loadOverrides, useRootOverrides }) { if (!real) { - return realpath(path, this[_rpcache], this[_stcache]) - .then( - real => this[_loadFSNode]({ - path, - parent, - real, - root, - loadOverrides, - useRootOverrides, - }), - // if realpath fails, just provide a dummy error node - error => new Node({ - error, - path, - realpath: path, - parent, - root, - loadOverrides, - }) - ) - } - - // cache temporarily holds a promise placeholder so we don't try to create - // the same node multiple times. this is rare to encounter, given the - // aggressive caching on realpath and lstat calls, but it's possible that - // it's already loaded as a tree top, and then gets its parent loaded - // later, if a symlink points deeper in the tree. - const cached = this[_cache].get(path) - if (cached && !cached.dummy) { - return Promise.resolve(cached).then(node => { - node.parent = parent - return node - }) - } - - const p = rpj(join(real, 'package.json')) - // soldier on if read-package-json raises an error - .then(pkg => [pkg, null], error => [null, error]) - .then(([pkg, error]) => { - return this[normalize(path) === real ? _newNode : _newLink]({ - installLinks: this.installLinks, - legacyPeerDeps: this.legacyPeerDeps, - path, - realpath: real, - pkg, + try { + real = await realpath(path, this[_rpcache], this[_stcache]) + } catch (error) { + // if realpath fails, just provide a dummy error node + return new Node({ error, + path, + realpath: path, parent, root, loadOverrides, - ...(useRootOverrides && root.overrides - ? { overrides: root.overrides.getNodeRule({ name: pkg.name, version: pkg.version }) } - : {}), }) - }) - .then(node => { - this[_cache].set(path, node) - return node - }) + } + } + + const cached = this[_cache].get(path) + let node + // missing edges get a dummy node, assign the parent and return it + if (cached && !cached.dummy) { + cached.parent = parent + return cached + } else { + const params = { + installLinks: this.installLinks, + legacyPeerDeps: this.legacyPeerDeps, + path, + realpath: real, + parent, + root, + loadOverrides, + } + + try { + const pkg = await rpj(join(real, 'package.json')) + params.pkg = pkg + if (useRootOverrides && root.overrides) { + params.overrides = root.overrides.getNodeRule({ name: pkg.name, version: pkg.version }) + } + } catch (err) { + params.error = err + } - this[_cache].set(path, p) - return p + // soldier on if read-package-json raises an error, passing it to the + // Node which will attach it to its errors array (Link passes it along to + // its target node) + if (normalize(path) === real) { + node = this[_newNode](params) + } else { + node = await this[_newLink](params) + } + } + this[_cache].set(path, node) + return node } - // this is the way it is to expose a timing issue which is difficult to - // test otherwise. The creation of a Node may take slightly longer than - // the creation of a Link that targets it. If the Node has _begun_ its - // creation phase (and put a Promise in the cache) then the Link will - // get a Promise as its cachedTarget instead of an actual Node object. - // This is not a problem, because it gets resolved prior to returning - // the tree or attempting to load children. However, it IS remarkably - // difficult to get to happen in a test environment to verify reliably. - // Hence this kludge. [_newNode] (options) { // check it for an fsParent if it's a tree top. there's a decent chance // it'll get parented later, making the fsParent scan a no-op, but better @@ -330,70 +324,56 @@ module.exports = cls => class ActualLoader extends cls { if (!parent) { this[_topNodes].add(realpath) } - return process.env._TEST_ARBORIST_SLOW_LINK_TARGET_ === '1' - ? new Promise(res => setTimeout(() => res(new Node(options)), 100)) - : new Node(options) + return new Node(options) } - [_newLink] (options) { + async [_newLink] (options) { const { realpath } = options this[_topNodes].add(realpath) const target = this[_cache].get(realpath) const link = new Link({ ...options, target }) if (!target) { + // Link set its target itself in this case this[_cache].set(realpath, link.target) // if a link target points at a node outside of the root tree's // node_modules hierarchy, then load that node as well. - return this[_loadFSTree](link.target).then(() => link) - } else if (target.then) { - // eslint-disable-next-line promise/catch-or-return - target.then(node => link.target = node) + await this[_loadFSTree](link.target) } return link } - [_loadFSTree] (node) { + async [_loadFSTree] (node) { const did = this[_actualTreeLoaded] - node = node.target - - // if a Link target has started, but not completed, then - // a Promise will be in the cache to indicate this. - if (node.then) { - return node.then(node => this[_loadFSTree](node)) - } - - // impossible except in pathological ELOOP cases - /* istanbul ignore if */ - if (did.has(node.realpath)) { - return Promise.resolve(node) - } - - did.add(node.realpath) - return this[_loadFSChildren](node) - .then(() => Promise.all( - [...node.children.entries()] + if (!did.has(node.target.realpath)) { + did.add(node.target.realpath) + await this[_loadFSChildren](node.target) + return Promise.all( + [...node.target.children.entries()] .filter(([name, kid]) => !did.has(kid.realpath)) - .map(([name, kid]) => this[_loadFSTree](kid)))) + .map(([name, kid]) => this[_loadFSTree](kid)) + ) + } } // create child nodes for all the entries in node_modules // and attach them to the node as a parent - [_loadFSChildren] (node) { + async [_loadFSChildren] (node) { const nm = resolve(node.realpath, 'node_modules') - return readdir(nm).then(kids => { + try { + const kids = await readdir(nm) return Promise.all( - // ignore . dirs and retired scoped package folders + // ignore . dirs and retired scoped package folders kids.filter(kid => !/^(@[^/]+\/)?\./.test(kid)) .filter(kid => this[_filter](node, kid)) .map(kid => this[_loadFSNode]({ parent: node, path: resolve(nm, kid), }))) - }, - // error in the readdir is not fatal, just means no kids - () => {}) + } catch { + // error in the readdir is not fatal, just means no kids + } } async [_findMissingEdges] () { @@ -440,6 +420,7 @@ module.exports = cls => class ActualLoader extends cls { const d = this[_cache].has(p) ? await this[_cache].get(p) : new Node({ path: p, root: node.root, dummy: true }) + // not a promise this[_cache].set(p, d) if (d.dummy) { // it's a placeholder, so likely would not have loaded this dep, @@ -460,22 +441,4 @@ module.exports = cls => class ActualLoader extends cls { await Promise.all(depPromises) } } - - // try to find a node that is the parent in a fs tree sense, but not a - // node_modules tree sense, of any link targets. this allows us to - // resolve deps that node will find, but a legacy npm view of the - // world would not have noticed. - [_findFSParents] () { - for (const path of this[_topNodes]) { - const node = this[_cache].get(path) - if (node && !node.parent && !node.fsParent) { - for (const p of walkUp(dirname(path))) { - if (this[_cache].has(p)) { - node.fsParent = this[_cache].get(p) - break - } - } - } - } - } }