From 53267a744d067b05969d8ebb5ceff0faa960035c Mon Sep 17 00:00:00 2001 From: Bronley Date: Thu, 8 Apr 2021 06:46:03 -0400 Subject: [PATCH 1/2] Prevent circular import crash --- src/DependencyGraph.spec.ts | 13 +++++++++---- src/DependencyGraph.ts | 33 ++++++++++++++++++++++----------- src/Scope.ts | 9 ++++++--- src/files/XmlFile.ts | 2 +- 4 files changed, 38 insertions(+), 19 deletions(-) diff --git a/src/DependencyGraph.spec.ts b/src/DependencyGraph.spec.ts index fd3c5b260..c51dabfee 100644 --- a/src/DependencyGraph.spec.ts +++ b/src/DependencyGraph.spec.ts @@ -142,10 +142,15 @@ describe('DependencyGraph', () => { expect(mock.callCount).to.equal(1); }); - it('emits immediately when configured to do so', () => { - let mock = sinon.mock(); - graph.onchange('a', mock, true); - expect(mock.callCount).to.equal(1); + it('does not cause infinite loop on circular dependency', () => { + //direct + graph.addOrReplace('a', ['b']); + graph.addOrReplace('b', ['a']); + + //indirect + graph.addOrReplace('c', ['d']); + graph.addOrReplace('d', ['e']); + graph.addOrReplace('e', ['c']); }); }); diff --git a/src/DependencyGraph.ts b/src/DependencyGraph.ts index ebb6bae90..081492412 100644 --- a/src/DependencyGraph.ts +++ b/src/DependencyGraph.ts @@ -9,7 +9,12 @@ export class DependencyGraph { */ public nodes = {} as Record; - private onchangeEmitter = new EventEmitter(); + /** + * An internal event emitter for when keys have changed. + * the `notified` property of the event object is a list of every key that has already been notified, + * and is used to prevent infinite notification loops + */ + private onchangeEmitter = new EventEmitter(); /** * Add a node to the graph. @@ -26,7 +31,7 @@ export class DependencyGraph { //create a new dependency node let node = new Node(key, dependencies, this); this.nodes[key] = node; - this.onchangeEmitter.emit(key, key); + this.emit(key, { sourceKey: key, notifiedKeys: new Set() }); } /** @@ -70,25 +75,26 @@ export class DependencyGraph { */ public remove(key: string) { delete this.nodes[key]; - this.onchangeEmitter.emit(key, key); + this.emit(key, { sourceKey: key, notifiedKeys: new Set() }); } /** * Emit event that this item has changed */ - public emit(key: string) { - this.onchangeEmitter.emit(key, key); + public emit(key: string, event: DependencyChangedEvent) { + //prevent infinite event loops by skipping already-notified keys + if (!event.notifiedKeys.has(key)) { + event.notifiedKeys.add(key); + this.onchangeEmitter.emit(key, event); + } } /** * Listen for any changes to dependencies with the given key. * @param emitImmediately if true, the handler will be called once immediately. */ - public onchange(key: string, handler: (key) => void, emitImmediately = false) { + public onchange(key: string, handler: (event: DependencyChangedEvent) => void) { this.onchangeEmitter.on(key, handler); - if (emitImmediately) { - this.onchangeEmitter.emit(key, key); - } return () => { this.onchangeEmitter.off(key, handler); }; @@ -103,6 +109,11 @@ export class DependencyGraph { } } +export interface DependencyChangedEvent { + sourceKey: string; + notifiedKeys: Set; +} + export class Node { public constructor( public key: string, @@ -113,9 +124,9 @@ export class Node { this.subscriptions = []; } for (let dependency of this.dependencies) { - let sub = this.graph.onchange(dependency, () => { + let sub = this.graph.onchange(dependency, (event) => { //notify the graph that we changed since one of our dependencies changed - this.graph.emit(this.key); + this.graph.emit(this.key, event); }); this.subscriptions.push(sub); diff --git a/src/Scope.ts b/src/Scope.ts index c3a9b2787..35466901f 100644 --- a/src/Scope.ts +++ b/src/Scope.ts @@ -16,6 +16,7 @@ import { URI } from 'vscode-uri'; import { LogLevel } from './Logger'; import { isBrsFile, isClassStatement, isFunctionStatement, isFunctionType, isXmlFile, isCustomType, isClassMethodStatement } from './astUtils/reflection'; import type { BrsFile } from './files/BrsFile'; +import type { DependencyChangedEvent } from './DependencyGraph'; /** * A class to keep track of all declarations within a given scope (like source scope, component scope) @@ -32,8 +33,10 @@ export class Scope { //anytime a dependency for this scope changes, we need to be revalidated this.programHandles.push( - this.program.dependencyGraph.onchange(this.dependencyGraphKey, this.onDependenciesChanged.bind(this), true) + this.program.dependencyGraph.onchange(this.dependencyGraphKey, this.onDependenciesChanged.bind(this)) ); + //invalidate immediately since this is a new scope + this.invalidate(); } /** @@ -119,8 +122,8 @@ export class Scope { */ protected diagnostics = [] as BsDiagnostic[]; - protected onDependenciesChanged(key: string) { - this.logDebug('invalidated because dependency graph said [', key, '] changed'); + protected onDependenciesChanged(event: DependencyChangedEvent) { + this.logDebug('invalidated because dependency graph said [', event.sourceKey, '] changed'); this.invalidate(); } diff --git a/src/files/XmlFile.ts b/src/files/XmlFile.ts index ddb681d35..c74d93843 100644 --- a/src/files/XmlFile.ts +++ b/src/files/XmlFile.ts @@ -489,7 +489,7 @@ export class XmlFile { if (this.needsTranspiled || extraImportScripts.length > 0) { //temporarily add the missing imports as script tags - const originalScripts = this.ast.component.scripts; + const originalScripts = this.ast.component?.scripts ?? []; this.ast.component.scripts = [ ...originalScripts, ...extraImportScripts From 17dd04366d0c00ff921f8799d06523349ed78ae9 Mon Sep 17 00:00:00 2001 From: Bronley Date: Thu, 8 Apr 2021 07:24:26 -0400 Subject: [PATCH 2/2] tweak comments --- src/DependencyGraph.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/DependencyGraph.ts b/src/DependencyGraph.ts index 081492412..889414a67 100644 --- a/src/DependencyGraph.ts +++ b/src/DependencyGraph.ts @@ -11,8 +11,6 @@ export class DependencyGraph { /** * An internal event emitter for when keys have changed. - * the `notified` property of the event object is a list of every key that has already been notified, - * and is used to prevent infinite notification loops */ private onchangeEmitter = new EventEmitter(); @@ -110,7 +108,13 @@ export class DependencyGraph { } export interface DependencyChangedEvent { + /** + * The key that was the initiator of this event. Child keys will emit this same event object, but this key will remain the same + */ sourceKey: string; + /** + * A set of keys that have already been notified of this change. Used to prevent circular reference notification cycles + */ notifiedKeys: Set; }