Merge pull request #27 from rokucommunity/fix-languageserver-race-con…

…dition

Fix languageserver race condition

CHANGELOG.md

@@ -6,6 +6,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 
 
+## [0.2.2] - 2019-09-27
+### Fixed
+ - bug in language server where the server would crash when sending a diagnostic too early. Now the server waits for the program to load before sending diagnostics.
+
+
+
 ## [0.2.1] - 2019-09-24
 ### Changed
  - the text for diagnostic 1010 to say "override" instead of "shadows"
@@ -38,6 +44,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 
 
+[0.2.2]:  https://github.com/rokucommunity/brighterscript/compare/v0.2.1...v0.2.2
 [0.2.1]:  https://github.com/rokucommunity/brighterscript/compare/v0.2.0...v0.2.1
 [0.2.0]:  https://github.com/rokucommunity/brighterscript/compare/v0.1.0...v0.2.0
 [0.1.0]:  https://github.com/rokucommunity/brighterscript/compare/v0.1.0...v0.1.0

package.json

@@ -1,6 +1,6 @@
 {
   "name": "brighterscript",
-  "version": "0.2.1",
+  "version": "0.2.2",
   "description": "A superset of Roku's BrightScript language.",
   "scripts": {
     "preversion": "npm run build && npm run tslint && npm run test",

readme.md

@@ -4,7 +4,7 @@ A superset of Roku's BrightScript language. Compiles to standard BrightScript.
 
 [![Build Status](https://travis-ci.org/rokucommunity/brighterscript.svg?branch=master)](https://travis-ci.org/rokucommunity/brighterscript)
 [![codecov](https://codecov.io/gh/rokucommunity/brighterscript/branch/master/graph/badge.svg)](https://codecov.io/gh/rokucommunity/brighterscript)
-[![NPM Version](https://badge.fury.io/js/brighterscript.svg?style=flat)](https://npmjs.org/package/@rokucommunity/brighterscript)
+[![NPM Version](https://badge.fury.io/js/brighterscript.svg?style=flat)](https://npmjs.org/package/brighterscript)
 
 The goal of this project is to bring new features and syntax enhancements to Roku's BrightScript language. It also supports parsing and validating standard BrightScript, so you don't even need to write a single line of `BrighterScript` code to benefit from this project. 
 

src/LanguageServer.spec.ts

@@ -12,11 +12,14 @@ afterEach(() => {
     sinon.restore();
 });
 
+import { Deferred } from './deferred';
 import { LanguageServer } from './LanguageServer';
+import { getFileProtocolPath } from './ProgramBuilder.spec';
+import util from './util';
 let rootDir = process.cwd();
 let n = path.normalize;
 
-describe.skip('LanguageServer', () => {
+describe('LanguageServer', () => {
     let server: LanguageServer;
     //an any version of the server for easier private testing
     let s: any;
@@ -90,13 +93,48 @@ describe.skip('LanguageServer', () => {
         fsExtra.writeFileSync(pathAbsolute, contents);
     }
 
+    describe('sendDiagnostics', () => {
+        it('waits for program to finish loading before sending diagnostics', async () => {
+            s.onInitialize({
+                capabilities: {
+                    workspace: {
+                        workspaceFolders: true
+                    }
+                }
+            });
+            expect(s.clientHasWorkspaceFolderCapability).to.be.true;
+            await server.run();
+            let deferred = new Deferred();
+            let workspace: any = {
+                builder: {
+                    getDiagnostics: () => []
+                },
+                firstRunPromise: deferred.promise
+            };
+            //make a new not-completed workspace
+            server.workspaces.push(workspace);
+
+            //this call should wait for the builder to finish
+            let p = s.sendDiagnostics();
+            // await s.createWorkspaces(
+            await util.sleep(50);
+            //simulate the program being created
+            workspace.builder.program = {
+                files: {}
+            };
+            deferred.resolve();
+            await p;
+            //test passed because no exceptions were thrown
+        });
+    });
+
     describe('onDidChangeWatchedFiles', () => {
         let workspacePath = n(`${rootDir}/TestRokuApp`);
         let mainPath = n(`${workspacePath}/source/main.brs`);
 
         it('picks up new files', async () => {
             workspaceFolders = [{
-                uri: `file:///${workspacePath}`,
+                uri: getFileProtocolPath(workspacePath),
                 name: 'TestProject'
             }];
 
@@ -105,7 +143,7 @@ describe.skip('LanguageServer', () => {
                 capabilities: {
                 }
             });
-            writeToFs(mainPath, `sub main(): return : end sub`);
+            writeToFs(mainPath, `sub main(): return: end sub`);
             await s.onInitialized();
             expect(server.workspaces[0].builder.program.hasFile(mainPath)).to.be.true;
             //move a file into the directory...the program should detect it
@@ -114,11 +152,11 @@ describe.skip('LanguageServer', () => {
 
             await s.onDidChangeWatchedFiles({
                 changes: [{
-                    uri: `file:///${libPath}`,
+                    uri: getFileProtocolPath(libPath),
                     type: 1 //created
                 },
                 {
-                    uri: `file:///${n(workspacePath + '/source')}`,
+                    uri: getFileProtocolPath(path.join(workspacePath, 'source')),
                     type: 2 //changed
                 }
                     // ,{

src/LanguageServer.ts

@@ -169,7 +169,7 @@ export class LanguageServer {
                 });
             }
             await this.waitAllProgramFirstRuns();
-            this.sendDiagnostics();
+            await this.sendDiagnostics();
         } catch (e) {
             this.sendCriticalFailure(
                 `Critical failure during BrighterScript language server startup.
@@ -335,7 +335,7 @@ export class LanguageServer {
                     severity: 'warning',
                     ...diagnosticMessages.BrsConfigJson_is_depricated_1020(),
                 });
-                this.sendDiagnostics();
+                return this.sendDiagnostics();
             }
         });
     }
@@ -425,7 +425,7 @@ export class LanguageServer {
         );
 
         //send all diagnostics
-        this.sendDiagnostics();
+        await this.sendDiagnostics();
     }
 
     private async onDidChangeConfiguration() {
@@ -474,7 +474,7 @@ export class LanguageServer {
         );
 
         //send all diagnostics to the client
-        this.sendDiagnostics();
+        await this.sendDiagnostics();
         this.connection.sendNotification('build-status', 'success');
     }
 
@@ -528,7 +528,7 @@ export class LanguageServer {
         } catch (e) {
             this.sendCriticalFailure(`Critical error parsing/validating ${filePath}: ${e.message}`);
         }
-        this.sendDiagnostics();
+        await this.sendDiagnostics();
         this.connection.sendNotification('build-status', 'success');
     }
 
@@ -554,13 +554,14 @@ export class LanguageServer {
      * diagnostics to send and which to skip because nothing has changed
      */
     private latestDiagnosticsByFile = {} as { [key: string]: Diagnostic[] };
-    private sendDiagnostics() {
+    private async sendDiagnostics() {
         //compute the new list of diagnostics for whole project
         let issuesByFile = {} as { [key: string]: Diagnostic[] };
-        // let uri = Uri.parse(textDocument.uri);
 
         //make a bucket for every file in every project
         for (let workspace of this.workspaces) {
+            //Ensure the program was constructued. This prevents race conditions where certain diagnostics are being sent before the program was created.
+            await workspace.firstRunPromise;
             for (let filePath in workspace.builder.program.files) {
                 issuesByFile[filePath] = [];
             }

src/ProgramBuilder.spec.ts

@@ -70,15 +70,6 @@ describe('ProgramBuilder', () => {
          * Error: [UriError]: If a URI does not contain an authority component, then the path cannot begin with two slash characters ("//")
          * @param fullPath
          */
-        function getFileProtocolPath(fullPath: string) {
-            let result: string;
-            if (fullPath.indexOf('/') === 0 || fullPath.indexOf('\\') === 0) {
-                result = `file://${fullPath}`;
-            } else {
-                result = `file:///${fullPath}`;
-            }
-            return result;
-        }
 
         it('only adds files that match the files array', async () => {
             sinon.stub(util, 'getFilePaths').returns(
@@ -121,3 +112,13 @@ describe('ProgramBuilder', () => {
         });
     });
 });
+
+export function getFileProtocolPath(fullPath: string) {
+    let result: string;
+    if (fullPath.indexOf('/') === 0 || fullPath.indexOf('\\') === 0) {
+        result = `file://${fullPath}`;
+    } else {
+        result = `file:///${fullPath}`;
+    }
+    return result;
+}

src/util.ts

@@ -671,6 +671,16 @@ export class Util {
         }
         return tokens;
     }
+
+    /**
+     * Set a timeout for the specified milliseconds, and resolve the promise once the timeout is finished.
+     * @param milliseconds
+     */
+    public sleep(milliseconds: number) {
+        return new Promise((resolve) => {
+            setTimeout(resolve, milliseconds);
+        });
+    }
     /**
      * The BRS library uses 1-based line indexes, and 0 based column indexes.
      * However, vscode expects zero-based for everything.