From 5b45e1240590c1582c8bf6cfb3ee0fa5994e3770 Mon Sep 17 00:00:00 2001 From: Anton Kosyakov Date: Tue, 3 Dec 2019 10:56:18 +0000 Subject: [PATCH] [tree]: fix infinity recursion when tree root is change during ongoing refresh The navigator model root gets initialized too eagerly before the workspace service is ready to provide roots. And then the second time by an event then the workspace service is initialized. It causes 2 parallel refreshes and with bad timing on workspaces with multiple roots can lead to infinity loop and eventually max call stack error. This resolves the issue generically for trees by allowing parallel refreshes without resetting a root. Signed-off-by: Anton Kosyakov --- .../browser/tree/test/tree-test-container.ts | 45 ++++ .../src/browser/tree/tree-consistency.spec.ts | 101 ++++++++ .../src/browser/tree/tree-expansion.spec.ts | 216 ++++++++---------- .../core/src/browser/tree/tree-expansion.ts | 23 +- packages/core/src/browser/tree/tree-model.ts | 19 +- packages/core/src/browser/tree/tree.spec.ts | 31 +-- packages/core/src/browser/tree/tree.ts | 33 +-- .../navigator/src/browser/navigator-model.ts | 8 +- 8 files changed, 294 insertions(+), 182 deletions(-) create mode 100644 packages/core/src/browser/tree/test/tree-test-container.ts create mode 100644 packages/core/src/browser/tree/tree-consistency.spec.ts diff --git a/packages/core/src/browser/tree/test/tree-test-container.ts b/packages/core/src/browser/tree/test/tree-test-container.ts new file mode 100644 index 0000000000000..bcb44d56027bd --- /dev/null +++ b/packages/core/src/browser/tree/test/tree-test-container.ts @@ -0,0 +1,45 @@ +/******************************************************************************** + * Copyright (C) 2019 TypeFox and others. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0 which is available at + * http://www.eclipse.org/legal/epl-2.0. + * + * This Source Code may also be made available under the following Secondary + * Licenses when the conditions for such availability set forth in the Eclipse + * Public License v. 2.0 are satisfied: GNU General Public License, version 2 + * with the GNU Classpath Exception which is available at + * https://www.gnu.org/software/classpath/license.html. + * + * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 + ********************************************************************************/ + +import { TreeImpl, Tree } from '../tree'; +import { TreeModel, TreeModelImpl } from '../tree-model'; +import { Container } from 'inversify'; +import { TreeSelectionServiceImpl } from '../tree-selection-impl'; +import { TreeSelectionService } from '../tree-selection'; +import { TreeExpansionServiceImpl, TreeExpansionService } from '../tree-expansion'; +import { TreeNavigationService } from '../tree-navigation'; +import { TreeSearch } from '../tree-search'; +import { FuzzySearch } from '../fuzzy-search'; +import { MockLogger } from '../../../common/test/mock-logger'; +import { ILogger } from '../../../common'; + +export function createTreeTestContainer(): Container { + const container = new Container({ defaultScope: 'Singleton' }); + container.bind(TreeImpl).toSelf(); + container.bind(Tree).toService(TreeImpl); + container.bind(TreeSelectionServiceImpl).toSelf(); + container.bind(TreeSelectionService).toService(TreeSelectionServiceImpl); + container.bind(TreeExpansionServiceImpl).toSelf(); + container.bind(TreeExpansionService).toService(TreeExpansionServiceImpl); + container.bind(TreeNavigationService).toSelf(); + container.bind(TreeModelImpl).toSelf(); + container.bind(TreeModel).toService(TreeModelImpl); + container.bind(TreeSearch).toSelf(); + container.bind(FuzzySearch).toSelf(); + container.bind(MockLogger).toSelf(); + container.bind(ILogger).to(MockLogger); + return container; +} diff --git a/packages/core/src/browser/tree/tree-consistency.spec.ts b/packages/core/src/browser/tree/tree-consistency.spec.ts new file mode 100644 index 0000000000000..c77e3ce4215da --- /dev/null +++ b/packages/core/src/browser/tree/tree-consistency.spec.ts @@ -0,0 +1,101 @@ +/******************************************************************************** + * Copyright (C) 2019 TypeFox and others. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0 which is available at + * http://www.eclipse.org/legal/epl-2.0. + * + * This Source Code may also be made available under the following Secondary + * Licenses when the conditions for such availability set forth in the Eclipse + * Public License v. 2.0 are satisfied: GNU General Public License, version 2 + * with the GNU Classpath Exception which is available at + * https://www.gnu.org/software/classpath/license.html. + * + * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 + ********************************************************************************/ + +import * as assert from 'assert'; +import { injectable } from 'inversify'; +import { createTreeTestContainer } from './test/tree-test-container'; +import { TreeImpl, CompositeTreeNode, TreeNode } from './tree'; +import { TreeModel } from './tree-model'; +import { ExpandableTreeNode } from './tree-expansion'; + +@injectable() +class ConsistencyTestTree extends TreeImpl { + + public resolveCounter = 0; + + protected async resolveChildren(parent: CompositeTreeNode): Promise { + if (parent.id === 'expandable') { + const step: () => Promise = async () => { + // a predicate to emulate bad timing, i.e. + // children of a node gets resolved when a root is changed + if (this.root && this.root !== parent.parent) { + this.resolveCounter++; + return []; + } else { + await new Promise(resolve => setTimeout(resolve, 10)); + return step(); + } + }; + return step(); + } + return super.resolveChildren(parent); + } + +} + +/** + * Return roots having the same id, but not object identity. + */ +function createConsistencyTestRoot(rootName: string): CompositeTreeNode { + const children: TreeNode[] = []; + const root: CompositeTreeNode = { + id: 'root', + name: rootName, + parent: undefined, + children + }; + const parent: ExpandableTreeNode = { + id: 'expandable', + name: 'expandable', + parent: root, + expanded: true, + children: [] + }; + children.push(parent); + return root; +} + +describe('Tree Consistency', () => { + + it('setting different tree roots should finish', async () => { + const container = createTreeTestContainer(); + container.bind(ConsistencyTestTree).toSelf(); + container.rebind(TreeImpl).toService(ConsistencyTestTree); + const tree = container.get(ConsistencyTestTree); + + const model = container.get(TreeModel); + + model.root = createConsistencyTestRoot('Foo'); + await new Promise(resolve => setTimeout(resolve, 50)); + + model.root = createConsistencyTestRoot('Bar'); + await new Promise(resolve => setTimeout(resolve, 50)); + + let resolveCounter = tree.resolveCounter; + assert.deepStrictEqual(tree.resolveCounter, 1); + for (let i = 0; i < 10; i++) { + await new Promise(resolve => setTimeout(resolve, 50)); + if (resolveCounter === tree.resolveCounter) { + assert.deepStrictEqual(tree.resolveCounter, 1); + assert.deepStrictEqual(model.root!.name, 'Bar'); + return; + } + resolveCounter = tree.resolveCounter; + } + assert.ok(false, 'Resolving does not stop, attempts: ' + tree.resolveCounter); + }); + +}); diff --git a/packages/core/src/browser/tree/tree-expansion.spec.ts b/packages/core/src/browser/tree/tree-expansion.spec.ts index 103e1c6939a33..fb48441aa2bac 100644 --- a/packages/core/src/browser/tree/tree-expansion.spec.ts +++ b/packages/core/src/browser/tree/tree-expansion.spec.ts @@ -16,143 +16,123 @@ import { expect } from 'chai'; import { MockTreeModel } from './test/mock-tree-model'; -import { TreeModelImpl, TreeModel } from './tree-model'; -import { TreeImpl, Tree, TreeNode, CompositeTreeNode } from './tree'; -import { Container } from 'inversify'; -import { TreeSelectionServiceImpl } from './tree-selection-impl'; -import { TreeSelectionService } from './tree-selection'; -import { TreeExpansionServiceImpl, TreeExpansionService, ExpandableTreeNode } from './tree-expansion'; -import { TreeNavigationService } from './tree-navigation'; -import { TreeSearch } from './tree-search'; -import { FuzzySearch } from './fuzzy-search'; -import { MockLogger } from '../../common/test/mock-logger'; -import { ILogger } from '../../common'; +import { TreeModel } from './tree-model'; +import { TreeNode, CompositeTreeNode } from './tree'; +import { ExpandableTreeNode } from './tree-expansion'; +import { createTreeTestContainer } from './test/tree-test-container'; // tslint:disable:no-unused-expression describe('TreeExpansionService', () => { - let model: TreeModel; - beforeEach(() => { - model = createTreeModel(); - model.root = MockTreeModel.HIERARCHICAL_MOCK_ROOT(); - }); - describe('expandNode', () => { - it('won\'t expand an already expanded node', done => { - const node: ExpandableTreeNode = retrieveNode('1'); - model.expandNode(node).then(result => { - expect(result).to.be.false; - done(); - }); + let model: TreeModel; + beforeEach(() => { + model = createTreeModel(); + model.root = MockTreeModel.HIERARCHICAL_MOCK_ROOT(); }); + describe('expandNode', () => { + it("won't expand an already expanded node", done => { + const node: ExpandableTreeNode = retrieveNode('1'); + model.expandNode(node).then(result => { + expect(result).to.be.undefined; + done(); + }); + }); - it('will expand a collapsed node', done => { - const node: ExpandableTreeNode = retrieveNode('1'); - model.collapseNode(node).then(() => { - model.expandNode(node).then(result => { - expect(result).to.be.true; - done(); + it('will expand a collapsed node', done => { + const node: ExpandableTreeNode = retrieveNode('1'); + model.collapseNode(node).then(() => { + model.expandNode(node).then(result => { + expect(result).to.be.eq(result); + done(); + }); + }); }); - }); - }); - it('won\'t expand an undefined node', done => { - model.expandNode(undefined).then(result => { - expect(result).to.be.false; - done(); - }); + it("won't expand an undefined node", done => { + model.expandNode(undefined).then(result => { + expect(result).to.be.undefined; + done(); + }); + }); }); - }); - describe('collapseNode', () => { - it('will collapse an expanded node', done => { - const node: ExpandableTreeNode = retrieveNode('1'); - model.collapseNode(node).then(result => { - expect(result).to.be.true; - done(); - }); - }); + describe('collapseNode', () => { + it('will collapse an expanded node', done => { + const node: ExpandableTreeNode = retrieveNode('1'); + model.collapseNode(node).then(result => { + expect(result).to.be.eq(result); + done(); + }); + }); - it('won\'t collapse an already collapsed node', done => { - const node: ExpandableTreeNode = retrieveNode('1'); - model.collapseNode(node).then(() => { - model.collapseNode(node).then(result => { - expect(result).to.be.false; - done(); + it("won't collapse an already collapsed node", done => { + const node: ExpandableTreeNode = retrieveNode('1'); + model.collapseNode(node).then(() => { + model.collapseNode(node).then(result => { + expect(result).to.be.false; + done(); + }); + }); }); - }); - }); - it('cannot collapse a leaf node', done => { - const node: ExpandableTreeNode = retrieveNode('1.1.2'); - model.collapseNode(node).then(result => { - expect(result).to.be.false; - done(); - }); + it('cannot collapse a leaf node', done => { + const node: ExpandableTreeNode = retrieveNode('1.1.2'); + model.collapseNode(node).then(result => { + expect(result).to.be.false; + done(); + }); + }); }); - }); - describe('collapseAll', () => { - it('will collapse all nodes recursively', done => { - model.collapseAll(retrieveNode('1')).then(result => { - expect(result).to.be.true; - done(); - }); - }); + describe('collapseAll', () => { + it('will collapse all nodes recursively', done => { + model.collapseAll(retrieveNode('1')).then(result => { + expect(result).to.be.eq(result); + done(); + }); + }); - it('won\'t collapse nodes recursively if the root node is collapsed already', done => { - model.collapseNode(retrieveNode('1')).then(() => { - model.collapseAll(retrieveNode('1')).then(result => { - expect(result).to.be.true; - done(); + it("won't collapse nodes recursively if the root node is collapsed already", done => { + model.collapseNode(retrieveNode('1')).then(() => { + model.collapseAll(retrieveNode('1')).then(result => { + expect(result).to.be.eq(result); + done(); + }); + }); }); - }); }); - }); - describe('toggleNodeExpansion', () => { - it('changes the expansion state from expanded to collapsed', done => { - const node = retrieveNode('1'); - model.onExpansionChanged((e: Readonly) => { - expect(e).to.be.equal(node); - expect(e.expanded).to.be.false; - }); - model.toggleNodeExpansion(node).then(() => { - done(); - }); - }); + describe('toggleNodeExpansion', () => { + it('changes the expansion state from expanded to collapsed', done => { + const node = retrieveNode('1'); + model.onExpansionChanged((e: Readonly) => { + expect(e).to.be.equal(node); + expect(e.expanded).to.be.false; + }); + model.toggleNodeExpansion(node).then(() => { + done(); + }); + }); - it('changes the expansion state from collapsed to expanded', done => { - const node = retrieveNode('1'); - model.collapseNode(node).then(() => { - }); - model.onExpansionChanged((e: Readonly) => { - expect(e).to.be.equal(node); - expect(e.expanded).to.be.true; - }); - model.toggleNodeExpansion(node).then(() => { - done(); - }); + it('changes the expansion state from collapsed to expanded', done => { + const node = retrieveNode('1'); + model.collapseNode(node).then(() => { + }); + model.onExpansionChanged((e: Readonly) => { + expect(e).to.be.equal(node); + expect(e.expanded).to.be.true; + }); + model.toggleNodeExpansion(node).then(() => { + done(); + }); + }); }); - }); - function createTreeModel(): TreeModel { - const container = new Container({ defaultScope: 'Singleton' }); - container.bind(TreeImpl).toSelf(); - container.bind(Tree).toService(TreeImpl); - container.bind(TreeSelectionServiceImpl).toSelf(); - container.bind(TreeSelectionService).toService(TreeSelectionServiceImpl); - container.bind(TreeExpansionServiceImpl).toSelf(); - container.bind(TreeExpansionService).toService(TreeExpansionServiceImpl); - container.bind(TreeNavigationService).toSelf(); - container.bind(TreeModelImpl).toSelf(); - container.bind(TreeModel).toService(TreeModelImpl); - container.bind(TreeSearch).toSelf(); - container.bind(FuzzySearch).toSelf(); - container.bind(MockLogger).toSelf(); - container.bind(ILogger).to(MockLogger).inSingletonScope(); - return container.get(TreeModel); - } - function retrieveNode(id: string): Readonly { - const readonlyNode: Readonly = model.getNode(id) as T; - return readonlyNode; - } + function createTreeModel(): TreeModel { + const container = createTreeTestContainer(); + return container.get(TreeModel); + } + function retrieveNode(id: string): Readonly { + const readonlyNode: Readonly = model.getNode(id) as T; + return readonlyNode; + } }); diff --git a/packages/core/src/browser/tree/tree-expansion.ts b/packages/core/src/browser/tree/tree-expansion.ts index c1e2de27fcb94..472d109853f04 100644 --- a/packages/core/src/browser/tree/tree-expansion.ts +++ b/packages/core/src/browser/tree/tree-expansion.ts @@ -29,12 +29,12 @@ export interface TreeExpansionService extends Disposable { */ readonly onExpansionChanged: Event>; /** - * If the given node is valid and collapsed then expand it. + * Expand a node for the given node id if it is valid and collapsed. * Expanding a node refreshes all its children. * - * Return true if a node has been expanded; otherwise false. + * Return a valid expanded refreshed node or `undefined` if such does not exist. */ - expandNode(node: Readonly): Promise; + expandNode(node: Readonly): Promise | undefined>; /** * If the given node is valid and expanded then collapse it. * @@ -107,19 +107,22 @@ export class TreeExpansionServiceImpl implements TreeExpansionService { this.onExpansionChangedEmitter.fire(node); } - async expandNode(raw: ExpandableTreeNode): Promise { + async expandNode(raw: ExpandableTreeNode): Promise { const node = this.tree.validateNode(raw); if (ExpandableTreeNode.isCollapsed(node)) { - return await this.doExpandNode(node); + return this.doExpandNode(node); } - return false; + return undefined; } - protected async doExpandNode(node: ExpandableTreeNode): Promise { + protected async doExpandNode(node: ExpandableTreeNode): Promise { node.expanded = true; - await this.tree.refresh(node); - this.fireExpansionChanged(node); - return true; + const refreshed = await this.tree.refresh(node); + if (ExpandableTreeNode.isExpanded(refreshed)) { + this.fireExpansionChanged(refreshed); + return refreshed; + } + return undefined; } async collapseNode(raw: ExpandableTreeNode): Promise { diff --git a/packages/core/src/browser/tree/tree-model.ts b/packages/core/src/browser/tree/tree-model.ts index 0553fdf865ebc..5df2d152ce8cc 100644 --- a/packages/core/src/browser/tree/tree-model.ts +++ b/packages/core/src/browser/tree/tree-model.ts @@ -33,7 +33,7 @@ export interface TreeModel extends Tree, TreeSelectionService, TreeExpansionServ * Expands the given node. If the `node` argument is `undefined`, then expands the currently selected tree node. * If multiple tree nodes are selected, expands the most recently selected tree node. */ - expandNode(node?: Readonly): Promise; + expandNode(node?: Readonly): Promise | undefined>; /** * Collapses the given node. If the `node` argument is `undefined`, then collapses the currently selected tree node. @@ -216,12 +216,11 @@ export class TreeModelImpl implements TreeModel, SelectionProvider): Promise { + async refresh(parent?: Readonly): Promise { if (parent) { - await this.tree.refresh(parent); - } else { - await this.tree.refresh(); + return this.tree.refresh(parent); } + return this.tree.refresh(); } // tslint:disable-next-line:typedef @@ -238,19 +237,19 @@ export class TreeModelImpl implements TreeModel, SelectionProvider): Promise { + async expandNode(raw?: Readonly): Promise { for (const node of raw ? [raw] : this.selectedNodes) { if (ExpandableTreeNode.is(node)) { - return await this.expansionService.expandNode(node); + return this.expansionService.expandNode(node); } } - return false; + return undefined; } async collapseNode(raw?: Readonly): Promise { for (const node of raw ? [raw] : this.selectedNodes) { if (ExpandableTreeNode.is(node)) { - return await this.expansionService.collapseNode(node); + return this.expansionService.collapseNode(node); } } return false; @@ -262,7 +261,7 @@ export class TreeModelImpl implements TreeModel, SelectionProvider { @@ -230,7 +222,7 @@ describe('Tree', () => { function assertTreeNode(expectation: string, node: TreeNode): void { // tslint:disable-next-line:no-any - assert.deepEqual(expectation, JSON.stringify(node, (key: keyof CompositeTreeNode, value: any) => { + assert.deepStrictEqual(expectation, JSON.stringify(node, (key: keyof CompositeTreeNode, value: any) => { if (key === 'parent' || key === 'previousSibling' || key === 'nextSibling') { return value && value.id; } @@ -239,20 +231,7 @@ describe('Tree', () => { } function createTreeModel(): TreeModel { - const container = new Container({ defaultScope: 'Singleton' }); - container.bind(TreeImpl).toSelf(); - container.bind(Tree).toService(TreeImpl); - container.bind(TreeSelectionServiceImpl).toSelf(); - container.bind(TreeSelectionService).toService(TreeSelectionServiceImpl); - container.bind(TreeExpansionServiceImpl).toSelf(); - container.bind(TreeExpansionService).toService(TreeExpansionServiceImpl); - container.bind(TreeNavigationService).toSelf(); - container.bind(TreeModelImpl).toSelf(); - container.bind(TreeModel).toService(TreeModelImpl); - container.bind(TreeSearch).toSelf(); - container.bind(FuzzySearch).toSelf(); - container.bind(MockLogger).toSelf(); - container.bind(ILogger).to(MockLogger).inSingletonScope(); + const container = createTreeTestContainer(); return container.get(TreeModel); } function retrieveNode(id: string): Readonly { diff --git a/packages/core/src/browser/tree/tree.ts b/packages/core/src/browser/tree/tree.ts index 0c7045d667d20..51e5dec302005 100644 --- a/packages/core/src/browser/tree/tree.ts +++ b/packages/core/src/browser/tree/tree.ts @@ -43,12 +43,16 @@ export interface Tree extends Disposable { validateNode(node: TreeNode | undefined): TreeNode | undefined; /** * Refresh children of the root node. + * + * Return a valid refreshed composite root or `undefined` if such does not exist. */ - refresh(): Promise; + refresh(): Promise | undefined>; /** - * Refresh children of the given node if it is valid. + * Refresh children of a node for the give node id if it is valid. + * + * Return a valid refreshed composite node or `undefined` if such does not exist. */ - refresh(parent: Readonly): Promise; + refresh(parent: Readonly): Promise | undefined>; /** * Emit when the children of the given node are refreshed. */ @@ -256,26 +260,33 @@ export class TreeImpl implements Tree { return this.getNode(id); } - async refresh(raw?: CompositeTreeNode): Promise { + async refresh(raw?: CompositeTreeNode): Promise { const parent = !raw ? this._root : this.validateNode(raw); + let result: CompositeTreeNode | undefined; if (CompositeTreeNode.is(parent)) { + result = parent; const children = await this.resolveChildren(parent); - await this.setChildren(parent, children); + result = await this.setChildren(parent, children); } - // FIXME: it should not be here - // if the idea was to support refreshing of all kind of nodes, then API should be adapted this.fireChanged(); + return result; } protected resolveChildren(parent: CompositeTreeNode): Promise { return Promise.resolve(Array.from(parent.children)); } - protected async setChildren(parent: CompositeTreeNode, children: TreeNode[]): Promise { + protected async setChildren(parent: CompositeTreeNode, children: TreeNode[]): Promise { + const root = this.getRootNode(parent); + if (this.nodes[root.id] && this.nodes[root.id] !== root) { + console.error(`Child node '${parent.id}' does not belong to this '${root.id}' tree.`); + return undefined; + } this.removeNode(parent); parent.children = children; this.addNode(parent); await this.fireNodeRefreshed(parent); + return parent; } protected removeNode(node: TreeNode | undefined): void { @@ -297,12 +308,6 @@ export class TreeImpl implements Tree { protected addNode(node: TreeNode | undefined): void { if (node) { - const root = this.getRootNode(node); - if (this.nodes[root.id] && this.nodes[root.id] !== root) { - console.debug('Child node does not belong to this tree. Resetting root.'); - this.root = root; - return; - } this.nodes[node.id] = node; } if (CompositeTreeNode.is(node)) { diff --git a/packages/navigator/src/browser/navigator-model.ts b/packages/navigator/src/browser/navigator-model.ts index df8b43db01175..7ae912eb4c64e 100644 --- a/packages/navigator/src/browser/navigator-model.ts +++ b/packages/navigator/src/browser/navigator-model.ts @@ -130,14 +130,14 @@ export class FileNavigatorModel extends FileTreeModel { if (!uri.path.isAbsolute) { return undefined; } - let node = await this.getNodeClosestToRootByUri(uri); + let node = this.getNodeClosestToRootByUri(uri); // success stop condition // we have to reach workspace root because expanded node could be inside collapsed one if (WorkspaceRootNode.is(node)) { if (ExpandableTreeNode.is(node)) { if (!node.expanded) { - await this.expandNode(node); + node = await this.expandNode(node); } return node; } @@ -155,10 +155,10 @@ export class FileNavigatorModel extends FileTreeModel { if (await this.revealFile(uri.parent)) { if (node === undefined) { // get node if it wasn't mounted into navigator tree before expansion - node = await this.getNodeClosestToRootByUri(uri); + node = this.getNodeClosestToRootByUri(uri); } if (ExpandableTreeNode.is(node) && !node.expanded) { - await this.expandNode(node); + node = await this.expandNode(node); } return node; }