Repository: rokucommunity/brighterscript
Pull request #1379 (2024-12-17 20:41:10)

Prevent simultaneous validate() for same project
Forces `validate()` to run one-at-a-time. This prevents race conditions with diagnostics not always being cleaned up correctly.


Patch:
diff --git a/src/Program.spec.ts b/src/Program.spec.ts
index 2c4d0b04..552eb5c6 100644
--- a/src/Program.spec.ts
+++ b/src/Program.spec.ts
@@ -13,7 +13,7 @@ import { URI } from 'vscode-uri';
 import PluginInterface from './PluginInterface';
 import type { FunctionStatement, PrintStatement } from './parser/Statement';
 import { EmptyStatement } from './parser/Statement';
-import { expectCompletionsExcludes, expectCompletionsIncludes, expectDiagnostics, expectHasDiagnostics, expectZeroDiagnostics, trim, trimMap } from './testHelpers.spec';
+import { expectCompletionsExcludes, expectCompletionsIncludes, expectDiagnostics, expectHasDiagnostics, expectThrows, expectThrowsAsync, expectZeroDiagnostics, trim, trimMap } from './testHelpers.spec';
 import { doesNotThrow } from 'assert';
 import { createVisitor, WalkMode } from './astUtils/visitors';
 import { isBrsFile } from './astUtils/reflection';
@@ -22,6 +22,7 @@ import type { AstEditor } from './astUtils/AstEditor';
 import { tempDir, rootDir, stagingDir } from './testHelpers.spec';
 import type { BsDiagnostic } from './interfaces';
 import { createLogger } from './logging';
+import { Scope } from './Scope';
 
 let sinon = sinonImport.createSandbox();
 
@@ -225,6 +226,28 @@ describe('Program', () => {
     });
 
     describe('validate', () => {
+        it('validate (sync) passes along inner exception', () => {
+            program.setFile('source/main.brs', ``);
+            sinon.stub(Scope.prototype, 'validate').callsFake(() => {
+                throw new Error('Scope crash');
+            });
+            expectThrows(() => {
+                program.validate();
+            }, 'Scope crash');
+        });
+
+        it('validate (async) passes along inner exception', async () => {
+            program.setFile('source/main.brs', ``);
+            sinon.stub(Scope.prototype, 'validate').callsFake(() => {
+                throw new Error('Scope crash');
+            });
+            await expectThrowsAsync(async () => {
+                await program.validate({
+                    async: true
+                });
+            }, 'Scope crash');
+        });
+
         it('retains expressions after validate', () => {
             const file = program.setFile<BrsFile>('source/main.bs', `
                 sub test()
diff --git a/src/Program.ts b/src/Program.ts
index eb780d83..60fffe98 100644
--- a/src/Program.ts
+++ b/src/Program.ts
@@ -30,6 +30,7 @@ import { CallExpressionInfo } from './bscPlugin/CallExpressionInfo';
 import { SignatureHelpUtil } from './bscPlugin/SignatureHelpUtil';
 import { DiagnosticSeverityAdjuster } from './DiagnosticSeverityAdjuster';
 import { Sequencer } from './common/Sequencer';
+import { Deferred } from './deferred';
 
 const startOfSourcePkgPath = `source${path.sep}`;
 const bslibNonAliasedRokuModulesPkgPath = s`source/roku_modules/rokucommunity_bslib/bslib.brs`;
@@ -662,6 +663,13 @@ export class Program {
      */
     private validationRunSequence = 0;
 
+    /**
+     * How many milliseconds can pass while doing synchronous operations in validate before we register a short timeout (i.e. yield to the event loop)
+     */
+    private validationMinSyncDuration = 150;
+
+    private validatePromise: Promise<void> | undefined;
+
     /**
      * Traverse the entire project, and validate all scopes
      */
@@ -671,17 +679,35 @@ export class Program {
     public validate(options?: { async?: boolean; cancellationToken?: CancellationToken }) {
         const timeEnd = this.logger.timeStart(LogLevel.log, `Validating project${(this.logger.logLevel as LogLevel) > LogLevel.log ? ` (run ${this.validationRunSequence++})` : ''}`);
 
+        let previousValidationPromise = this.validatePromise;
+        const deferred = new Deferred();
+
+        if (options?.async) {
+            //we're async, so create a new promise chain to resolve after this validation is done
+            this.validatePromise = Promise.resolve(previousValidationPromise).then(() => {
+                return deferred.promise;
+            });
+
+            //we are not async but there's a pending promise, then we cannot run this validation
+        } else if (previousValidationPromise !== undefined) {
+            throw new Error('Cannot run synchronous validation while an async validation is in progress');
+        }
+
         const sequencer = new Sequencer({
             name: 'program.validate',
-            async: options?.async ?? false,
             cancellationToken: options?.cancellationToken ?? new CancellationTokenSource().token,
-            //how many milliseconds can pass while doing synchronous operations before we register a short timeout
-            minSyncDuration: 150
+            minSyncDuration: this.validationMinSyncDuration
         });
 
         //this sequencer allows us to run in both sync and async mode, depending on whether options.async is enabled.
         //We use this to prevent starving the CPU during long validate cycles when running in a language server context
-        return sequencer
+        sequencer
+            .once(() => {
+                //if running in async mode, return the previous validation promise to ensure we're only running one at a time
+                if (options?.async) {
+                    return previousValidationPromise;
+                }
+            })
             .once(() => {
                 this.diagnostics = [];
                 this.plugins.emit('beforeProgramValidate', this);
@@ -720,7 +746,21 @@ export class Program {
             .onSuccess(() => {
                 timeEnd();
             })
-            .run();
+            .onComplete(() => {
+                //regardless of the success of the validation, mark this run as complete
+                deferred.resolve();
+                //clear the validatePromise which means we're no longer running a validation
+                this.validatePromise = undefined;
+            });
+
+        //run the sequencer in async mode if enabled
+        if (options?.async) {
+            return sequencer.run();
+
+            //run the sequencer in sync mode
+        } else {
+            return sequencer.runSync();
+        }
     }
 
     /**
diff --git a/src/common/Sequencer.spec.ts b/src/common/Sequencer.spec.ts
index 3915e6ad..b8b338ec 100644
--- a/src/common/Sequencer.spec.ts
+++ b/src/common/Sequencer.spec.ts
@@ -6,7 +6,7 @@ describe('Sequencer', () => {
     it('cancels when asked', () => {
         const cancellationTokenSource = new CancellationTokenSource();
         const values = [];
-        void new Sequencer({
+        new Sequencer({
             name: 'test',
             cancellationToken: cancellationTokenSource.token,
             minSyncDuration: 100
@@ -15,8 +15,62 @@ describe('Sequencer', () => {
             if (i === 2) {
                 cancellationTokenSource.cancel();
             }
-        }).run();
+        }).runSync();
 
         expect(values).to.eql([1, 2]);
     });
+
+    it('throws when returning a promise from runSync', () => {
+        let error;
+        try {
+            new Sequencer().once(() => {
+                return Promise.resolve();
+            }).runSync();
+        } catch (e) {
+            error = e;
+        }
+        expect(error?.message).to.eql(`Action returned a promise which is unsupported when running 'runSync'`);
+    });
+
+    it('waits for async actions to complete', async () => {
+        const values = [];
+        await new Sequencer().forEach([1, 2, 3], async (i) => {
+            await new Promise((resolve) => {
+                setTimeout(resolve, 10);
+            });
+            values.push(i);
+        }).run();
+
+        expect(values).to.eql([1, 2, 3]);
+    });
+
+    it('runSync() calls cancel before throwing', () => {
+        let cancelCalled = false;
+        try {
+            new Sequencer().once(() => {
+                throw new Error('crash');
+            }).onCancel(() => {
+                cancelCalled = true;
+            }).runSync();
+        } catch (e) {
+            //this is expected
+            expect((e as any).message).to.eql('crash');
+        }
+        expect(cancelCalled).to.be.true;
+    });
+
+    it('run() calls cancel before throwing', async () => {
+        let cancelCalled = false;
+        try {
+            await new Sequencer().once(() => {
+                throw new Error('crash');
+            }).onCancel(() => {
+                cancelCalled = true;
+            }).run();
+        } catch (e) {
+            //this is expected
+            expect((e as any).message).to.eql('crash');
+        }
+        expect(cancelCalled).to.be.true;
+    });
 });
diff --git a/src/common/Sequencer.ts b/src/common/Sequencer.ts
index d43b1a12..e7515c7f 100644
--- a/src/common/Sequencer.ts
+++ b/src/common/Sequencer.ts
@@ -7,19 +7,22 @@ import { EventEmitter } from 'eventemitter3';
  */
 export class Sequencer {
     constructor(
-        private options: {
-            name: string;
-            async?: boolean;
+        private options?: {
+            name?: string;
             cancellationToken?: CancellationToken;
             /**
              * The number of operations to run before registering a nexttick
              */
-            minSyncDuration: number;
+            minSyncDuration?: number;
         }
     ) {
 
     }
 
+    private get minSyncDuration() {
+        return this.options?.minSyncDuration ?? 150;
+    }
+
     // eslint-disable-next-line @typescript-eslint/ban-types
     private actions: Array<{ args: any[]; func: Function }> = [];
 
@@ -58,50 +61,50 @@ export class Sequencer {
         return this;
     }
 
-    /**
-     * Actually run the sequence
-     */
-    public run() {
-        if (this.options?.async) {
-            return this.runAsync();
-        } else {
-            return this.runSync();
-        }
-    }
-
-    private async runAsync() {
+    public async run() {
         try {
             let start = Date.now();
             for (const action of this.actions) {
                 //register a very short timeout between every action so we don't hog the CPU
-                if (Date.now() - start > this.options.minSyncDuration) {
+                if (Date.now() - start > this.minSyncDuration) {
                     await util.sleep(1);
                     start = Date.now();
                 }
 
                 //if the cancellation token has asked us to cancel, then stop processing now
-                if (this.options.cancellationToken?.isCancellationRequested) {
+                if (this.options?.cancellationToken?.isCancellationRequested) {
                     return this.handleCancel();
                 }
-                action.func(...action.args);
+                await Promise.resolve(
+                    action.func(...action.args)
+                );
             }
             this.emitter.emit('success');
+        } catch (e) {
+            this.handleCancel();
+            throw e;
         } finally {
             this.emitter.emit('complete');
             this.dispose();
         }
     }
 
-    private runSync() {
+    public runSync() {
         try {
             for (const action of this.actions) {
                 //if the cancellation token has asked us to cancel, then stop processing now
-                if (this.options.cancellationToken.isCancellationRequested) {
+                if (this.options?.cancellationToken?.isCancellationRequested) {
                     return this.handleCancel();
                 }
-                action.func(...action.args);
+                const result = action.func(...action.args);
+                if (typeof result?.then === 'function') {
+                    throw new Error(`Action returned a promise which is unsupported when running 'runSync'`);
+                }
             }
             this.emitter.emit('success');
+        } catch (e) {
+            this.handleCancel();
+            throw e;
         } finally {
             this.emitter.emit('complete');
             this.dispose();
@@ -109,7 +112,7 @@ export class Sequencer {
     }
 
     private handleCancel() {
-        console.log(`Cancelling sequence ${this.options.name}`);
+        console.log(`Cancelling sequence ${this.options?.name}`);
         this.emitter.emit('cancel');
     }
 
diff --git a/src/lsp/Project.spec.ts b/src/lsp/Project.spec.ts
index 3298f5c2..afed6729 100644
--- a/src/lsp/Project.spec.ts
+++ b/src/lsp/Project.spec.ts
@@ -1,11 +1,12 @@
 import { expect } from 'chai';
-import { tempDir, rootDir, expectDiagnosticsAsync, expectDiagnostics } from '../testHelpers.spec';
+import { tempDir, rootDir, expectDiagnosticsAsync, expectDiagnostics, once } from '../testHelpers.spec';
 import * as fsExtra from 'fs-extra';
 import { standardizePath as s } from '../util';
 import { Deferred } from '../deferred';
 import { DiagnosticMessages } from '../DiagnosticMessages';
 import { Project } from './Project';
 import { createSandbox } from 'sinon';
+import { Scope } from '../Scope';
 const sinon = createSandbox();
 
 describe('Project', () => {
@@ -40,6 +41,62 @@ describe('Project', () => {
         });
     });
 
+    describe('validate', () => {
+        it('prevents multiple valiations from running at the same time', async () => {
+            //create 10 scopes, which should each take at least 1ms to validate
+            for (let i = 0; i < 20; i++) {
+                fsExtra.outputFileSync(`${rootDir}/components/component${i}.xml`, `<component name="component${i}"></component>`);
+            }
+            await project.activate({
+                projectPath: rootDir,
+                workspaceFolder: rootDir,
+                enableThreading: false
+            });
+
+            //wait for the first validate to finish
+            const validate = project.validate;
+            await Promise.resolve((resolve) => {
+                const validateStub = sinon.stub(project, 'validate').callsFake((...args) => {
+                    return validate.call(project, ...args).then(() => {
+                        validateStub.restore();
+                        resolve();
+                    });
+                });
+            });
+
+            let validationCount = 0;
+            let maxValidationCount = 0;
+            //force validations to yield very frequently
+            project['builder'].program.plugins.add({
+                name: 'Test',
+                beforeProgramValidate: () => {
+                    validationCount++;
+                    maxValidationCount = Math.max(maxValidationCount, validationCount);
+                },
+                afterProgramValidate: () => {
+                    validationCount--;
+                }
+            });
+
+            //very small threshold so every validation step will yield
+            project['builder'].program['validationMinSyncDuration'] = 0.001;
+            sinon.stub(Scope.prototype, 'validate').callsFake(() => {
+                //each of these needs to take about 1ms to complete
+                const startTime = Date.now();
+                while (Date.now() - startTime < 2) { }
+            });
+
+            //validate 3 times in quick succession
+            await Promise.all([
+                project.validate(),
+                project.validate(),
+                project.validate()
+            ]);
+            expect(validationCount).to.eql(0);
+            expect(maxValidationCount).to.eql(1);
+        });
+    });
+
     describe('activate', () => {
         it('uses `files` from bsconfig.json', async () => {
             fsExtra.outputJsonSync(`${rootDir}/bsconfig.json`, {
@@ -57,6 +114,9 @@ describe('Project', () => {
             await project.activate({
                 projectPath: rootDir
             } as any);
+
+            await once(project, 'diagnostics');
+
             expectDiagnostics(project, [
                 DiagnosticMessages.cannotFindName('alpha').message
             ]);
@@ -159,6 +219,8 @@ describe('Project', () => {
                 projectPath: rootDir
             } as any);
 
+            await once(project, 'diagnostics');
+
             await expectDiagnosticsAsync(project, [
                 DiagnosticMessages.cannotFindName('varNotThere').message
             ]);


Test patch:
diff --git a/src/testHelpers.spec.ts b/src/testHelpers.spec.ts
index 926aa6e4..30c12703 100644
--- a/src/testHelpers.spec.ts
+++ b/src/testHelpers.spec.ts
@@ -471,3 +471,15 @@ export function createInactivityStub<T, K extends keyof T>(obj: T, methodName: k
         promise: inactivityPromise
     };
 }
+
+export async function once<T = any>(
+    obj: { on: (event, callback) => () => void },
+    event: string
+): Promise<T[]> {
+    return new Promise<T[]>((resolve) => {
+        const off = obj.on('diagnostics', (...args) => {
+            off();
+            resolve(args);
+        });
+    });
+}