Skip to content

Commit

Permalink
[tree]: fix infinity recursion when tree root is change during ongoin…
Browse files Browse the repository at this point in the history 
…g 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 <[email protected]>
  • Loading branch information
@akosyakov @svenefftinge
akosyakov authored and svenefftinge committed Dec 4, 2019
1 parent a551e6d commit 5b45e12
Show file tree
Hide file tree
Showing 8 changed files with 294 additions and 182 deletions.
45 changes: 45 additions & 0 deletions packages/core/src/browser/tree/test/tree-test-container.ts
View file
Original file line number Diff line number Diff line change
@@ -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;
}
101 changes: 101 additions & 0 deletions packages/core/src/browser/tree/tree-consistency.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -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<TreeNode[]> {
if (parent.id === 'expandable') {
const step: () => Promise<TreeNode[]> = 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>(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);
});

});
216 changes: 98 additions & 118 deletions packages/core/src/browser/tree/tree-expansion.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ExpandableTreeNode>('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<ExpandableTreeNode>('1');
model.expandNode(node).then(result => {
expect(result).to.be.undefined;
done();
});
});

it('will expand a collapsed node', done => {
const node: ExpandableTreeNode = retrieveNode<ExpandableTreeNode>('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<ExpandableTreeNode>('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<ExpandableTreeNode>('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<ExpandableTreeNode>('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<ExpandableTreeNode>('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<ExpandableTreeNode>('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<ExpandableTreeNode>('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<ExpandableTreeNode>('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<CompositeTreeNode>('1')).then(result => {
expect(result).to.be.true;
done();
});
});
describe('collapseAll', () => {
it('will collapse all nodes recursively', done => {
model.collapseAll(retrieveNode<CompositeTreeNode>('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<ExpandableTreeNode>('1')).then(() => {
model.collapseAll(retrieveNode<CompositeTreeNode>('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<ExpandableTreeNode>('1')).then(() => {
model.collapseAll(retrieveNode<CompositeTreeNode>('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<ExpandableTreeNode>('1');
model.onExpansionChanged((e: Readonly<ExpandableTreeNode>) => {
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<ExpandableTreeNode>('1');
model.onExpansionChanged((e: Readonly<ExpandableTreeNode>) => {
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<ExpandableTreeNode>('1');
model.collapseNode(node).then(() => {
});
model.onExpansionChanged((e: Readonly<ExpandableTreeNode>) => {
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<ExpandableTreeNode>('1');
model.collapseNode(node).then(() => {
});
model.onExpansionChanged((e: Readonly<ExpandableTreeNode>) => {
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<T extends TreeNode>(id: string): Readonly<T> {
const readonlyNode: Readonly<T> = model.getNode(id) as T;
return readonlyNode;
}
function createTreeModel(): TreeModel {
const container = createTreeTestContainer();
return container.get(TreeModel);
}
function retrieveNode<T extends TreeNode>(id: string): Readonly<T> {
const readonlyNode: Readonly<T> = model.getNode(id) as T;
return readonlyNode;
}
});
Loading

0 comments on commit 5b45e12

Please sign in to comment.