improve linter performance, kill older linters

DonJayamanne · DonJayamanne · commit 4a8319e0859f · 2017-02-10T21:25:35.000+11:00
diff --git a/src/client/common/utils.ts b/src/client/common/utils.ts
@@ -101,7 +101,7 @@ export function execPythonFile(file: string, args: string[], cwd: string, includ
         if (stdOut) {
             return spawnFileInternal(file, args, { cwd }, includeErrorAsResponse, stdOut, token);
         }
-        return execFileInternal(file, args, { cwd: cwd }, includeErrorAsResponse);
+        return execFileInternal(file, args, { cwd: cwd }, includeErrorAsResponse, token);
     }
 
     return getPythonInterpreterDirectory().then(pyPath => {
@@ -110,7 +110,7 @@ export function execPythonFile(file: string, args: string[], cwd: string, includ
             if (stdOut) {
                 return spawnFileInternal(file, args, { cwd }, includeErrorAsResponse, stdOut, token);
             }
-            return execFileInternal(file, args, { cwd: cwd }, includeErrorAsResponse);
+            return execFileInternal(file, args, { cwd: cwd }, includeErrorAsResponse, token);
         }
 
         if (customEnvVariables === null) {
@@ -134,7 +134,7 @@ export function execPythonFile(file: string, args: string[], cwd: string, includ
         if (stdOut) {
             return spawnFileInternal(file, args, { cwd, env: customEnvVariables }, includeErrorAsResponse, stdOut, token);
         }
-        return execFileInternal(file, args, { cwd, env: customEnvVariables }, includeErrorAsResponse);
+        return execFileInternal(file, args, { cwd, env: customEnvVariables }, includeErrorAsResponse, token);
     });
 }
 
@@ -160,11 +160,19 @@ function handleResponse(file: string, includeErrorAsResponse: boolean, error: Er
         resolve(stdout + '');
     });
 }
-function execFileInternal(file: string, args: string[], options: child_process.ExecFileOptions, includeErrorAsResponse: boolean): Promise<string> {
+function execFileInternal(file: string, args: string[], options: child_process.ExecFileOptions, includeErrorAsResponse: boolean, token?: CancellationToken): Promise<string> {
     return new Promise<string>((resolve, reject) => {
-        child_process.execFile(file, args, options, (error, stdout, stderr) => {
+        let proc = child_process.execFile(file, args, options, (error, stdout, stderr) => {
             handleResponse(file, includeErrorAsResponse, error, stdout, stderr).then(resolve, reject);
         });
+        if (token && token.onCancellationRequested) {
+            token.onCancellationRequested(() => {
+                if (proc) {
+                    proc.kill();
+                    proc = null;
+                }
+            });
+        }
     });
 }
 function spawnFileInternal(file: string, args: string[], options: child_process.ExecFileOptions, includeErrorAsResponse: boolean, stdOut: (line: string) => void, token?: CancellationToken): Promise<string> {
diff --git a/src/client/linters/baseLinter.ts b/src/client/linters/baseLinter.ts
@@ -63,13 +63,13 @@ export abstract class BaseLinter {
         this.pythonSettings = settings.PythonSettings.getInstance();
     }
     public abstract isEnabled(): Boolean;
-    public abstract runLinter(document: vscode.TextDocument): Promise<ILintMessage[]>;
+    public abstract runLinter(document: vscode.TextDocument, cancellation: vscode.CancellationToken): Promise<ILintMessage[]>;
 
-    protected run(command: string, args: string[], document: vscode.TextDocument, cwd: string, regEx: string = REGEX): Promise<ILintMessage[]> {
+    protected run(command: string, args: string[], document: vscode.TextDocument, cwd: string, cancellation: vscode.CancellationToken, regEx: string = REGEX): Promise<ILintMessage[]> {
         let outputChannel = this.outputChannel;
 
         return new Promise<ILintMessage[]>((resolve, reject) => {
-            execPythonFile(command, args, cwd, true).then(data => {
+            execPythonFile(command, args, cwd, true, null, cancellation).then(data => {
                 outputChannel.append('#'.repeat(10) + 'Linting Output - ' + this.Id + '#'.repeat(10) + '\n');
                 outputChannel.append(data);
                 let outputLines = data.split(/\r?\n/g);
diff --git a/src/client/linters/flake8.ts b/src/client/linters/flake8.ts
@@ -3,7 +3,7 @@
 import * as baseLinter from './baseLinter';
 import { OutputChannel } from 'vscode';
 import { Product } from '../common/installer';
-import { TextDocument } from 'vscode';
+import { TextDocument, CancellationToken } from 'vscode';
 
 export class Linter extends baseLinter.BaseLinter {
     constructor(outputChannel: OutputChannel, workspaceRootPath?: string) {
@@ -27,15 +27,15 @@ export class Linter extends baseLinter.BaseLinter {
     public isEnabled(): Boolean {
         return this.pythonSettings.linting.flake8Enabled;
     }
-    public runLinter(document: TextDocument): Promise<baseLinter.ILintMessage[]> {
+    public runLinter(document: TextDocument, cancellation: CancellationToken): Promise<baseLinter.ILintMessage[]> {
         if (!this.pythonSettings.linting.flake8Enabled) {
             return Promise.resolve([]);
         }
 
         let flake8Path = this.pythonSettings.linting.flake8Path;
         let flake8Args = Array.isArray(this.pythonSettings.linting.flake8Args) ? this.pythonSettings.linting.flake8Args : [];
         return new Promise<baseLinter.ILintMessage[]>((resolve, reject) => {
-            this.run(flake8Path, flake8Args.concat(['--format=%(row)d,%(col)d,%(code)s,%(code)s:%(text)s', document.uri.fsPath]), document, this.workspaceRootPath).then(messages => {
+            this.run(flake8Path, flake8Args.concat(['--format=%(row)d,%(col)d,%(code)s,%(code)s:%(text)s', document.uri.fsPath]), document, this.workspaceRootPath, cancellation).then(messages => {
                 messages.forEach(msg => {
                     msg.severity = this.parseMessagesCodeSeverity(msg.type);
                 });
diff --git a/src/client/linters/mypy.ts b/src/client/linters/mypy.ts
@@ -3,7 +3,7 @@
 import * as baseLinter from './baseLinter';
 import { OutputChannel } from 'vscode';
 import { Product } from '../common/installer';
-import { TextDocument } from 'vscode';
+import { TextDocument, CancellationToken } from 'vscode';
 
 const REGEX = '(?<file>.py):(?<line>\\d+): (?<type>\\w+): (?<message>.*)\\r?(\\n|$)';
 
@@ -28,15 +28,15 @@ export class Linter extends baseLinter.BaseLinter {
     public isEnabled(): Boolean {
         return this.pythonSettings.linting.mypyEnabled;
     }
-    public runLinter(document: TextDocument): Promise<baseLinter.ILintMessage[]> {
+    public runLinter(document: TextDocument, cancellation: CancellationToken): Promise<baseLinter.ILintMessage[]> {
         if (!this.pythonSettings.linting.mypyEnabled) {
             return Promise.resolve([]);
         }
 
         let mypyPath = this.pythonSettings.linting.mypyPath;
         let mypyArgs = Array.isArray(this.pythonSettings.linting.mypyArgs) ? this.pythonSettings.linting.mypyArgs : [];
         return new Promise<baseLinter.ILintMessage[]>((resolve, reject) => {
-            this.run(mypyPath, mypyArgs.concat([document.uri.fsPath]), document, this.workspaceRootPath, REGEX).then(messages => {
+            this.run(mypyPath, mypyArgs.concat([document.uri.fsPath]), document, this.workspaceRootPath, cancellation, REGEX).then(messages => {
                 messages.forEach(msg => {
                     msg.severity = this.parseMessagesSeverity(msg.type);
                     msg.code = msg.type;
diff --git a/src/client/linters/pep8Linter.ts b/src/client/linters/pep8Linter.ts
@@ -1,9 +1,9 @@
 'use strict';
 
 import * as baseLinter from './baseLinter';
-import {OutputChannel} from 'vscode';
+import { OutputChannel } from 'vscode';
 import { Product } from '../common/installer';
-import { TextDocument } from 'vscode';
+import { TextDocument, CancellationToken } from 'vscode';
 
 export class Linter extends baseLinter.BaseLinter {
     constructor(outputChannel: OutputChannel, workspaceRootPath?: string) {
@@ -26,15 +26,15 @@ export class Linter extends baseLinter.BaseLinter {
     public isEnabled(): Boolean {
         return this.pythonSettings.linting.pep8Enabled;
     }
-    public runLinter(document:TextDocument): Promise<baseLinter.ILintMessage[]> {
+    public runLinter(document: TextDocument, cancellation: CancellationToken): Promise<baseLinter.ILintMessage[]> {
         if (!this.pythonSettings.linting.pep8Enabled) {
             return Promise.resolve([]);
         }
 
         let pep8Path = this.pythonSettings.linting.pep8Path;
         let pep8Args = Array.isArray(this.pythonSettings.linting.pep8Args) ? this.pythonSettings.linting.pep8Args : [];
         return new Promise<baseLinter.ILintMessage[]>(resolve => {
-            this.run(pep8Path, pep8Args.concat(['--format=%(row)d,%(col)d,%(code)s,%(code)s:%(text)s', document.uri.fsPath]), document, this.workspaceRootPath).then(messages => {
+            this.run(pep8Path, pep8Args.concat(['--format=%(row)d,%(col)d,%(code)s,%(code)s:%(text)s', document.uri.fsPath]), document, this.workspaceRootPath, cancellation).then(messages => {
                 messages.forEach(msg => {
                     msg.severity = this.parseMessagesCodeSeverity(msg.type);
                 });
diff --git a/src/client/linters/prospector.ts b/src/client/linters/prospector.ts
@@ -1,10 +1,10 @@
 'use strict';
 
 import * as baseLinter from './baseLinter';
-import {OutputChannel} from 'vscode';
-import {execPythonFile} from './../common/utils';
+import { OutputChannel } from 'vscode';
+import { execPythonFile } from './../common/utils';
 import { Product } from '../common/installer';
-import { TextDocument } from 'vscode';
+import { TextDocument, CancellationToken } from 'vscode';
 
 interface IProspectorResponse {
     messages: IProspectorMessage[];
@@ -31,7 +31,7 @@ export class Linter extends baseLinter.BaseLinter {
     public isEnabled(): Boolean {
         return this.pythonSettings.linting.prospectorEnabled;
     }
-    public runLinter(document:TextDocument): Promise<baseLinter.ILintMessage[]> {
+    public runLinter(document: TextDocument, cancellation: CancellationToken): Promise<baseLinter.ILintMessage[]> {
         if (!this.pythonSettings.linting.prospectorEnabled) {
             return Promise.resolve([]);
         }
@@ -40,7 +40,7 @@ export class Linter extends baseLinter.BaseLinter {
         let outputChannel = this.outputChannel;
         let prospectorArgs = Array.isArray(this.pythonSettings.linting.prospectorArgs) ? this.pythonSettings.linting.prospectorArgs : [];
         return new Promise<baseLinter.ILintMessage[]>((resolve, reject) => {
-            execPythonFile(prospectorPath, prospectorArgs.concat(['--absolute-paths', '--output-format=json', document.uri.fsPath]), this.workspaceRootPath, false).then(data => {
+            execPythonFile(prospectorPath, prospectorArgs.concat(['--absolute-paths', '--output-format=json', document.uri.fsPath]), this.workspaceRootPath, false, null, cancellation).then(data => {
                 let parsedData: IProspectorResponse;
                 try {
                     parsedData = JSON.parse(data);
@@ -53,8 +53,8 @@ export class Linter extends baseLinter.BaseLinter {
                 let diagnostics: baseLinter.ILintMessage[] = [];
                 parsedData.messages.filter((value, index) => index <= this.pythonSettings.linting.maxNumberOfProblems).forEach(msg => {
 
-		    let lineNumber = msg.location.line === null || isNaN(msg.location.line) ? 1 : msg.location.line;
-		    let sourceLine = document.lineAt(lineNumber - 1).text;
+                    let lineNumber = msg.location.line === null || isNaN(msg.location.line) ? 1 : msg.location.line;
+                    let sourceLine = document.lineAt(lineNumber - 1).text;
                     let sourceStart = sourceLine.substring(msg.location.character);
 
                     // try to get the first word from the starting position
diff --git a/src/client/linters/pydocstyle.ts b/src/client/linters/pydocstyle.ts
@@ -6,7 +6,7 @@ import { ILintMessage } from './baseLinter';
 import { OutputChannel } from 'vscode';
 import { execPythonFile, IS_WINDOWS } from './../common/utils';
 import { Product } from '../common/installer';
-import { TextDocument } from 'vscode';
+import { TextDocument, CancellationToken } from 'vscode';
 
 export class Linter extends baseLinter.BaseLinter {
     constructor(outputChannel: OutputChannel, workspaceRootPath?: string) {
@@ -16,15 +16,15 @@ export class Linter extends baseLinter.BaseLinter {
     public isEnabled(): Boolean {
         return this.pythonSettings.linting.pydocstyleEnabled;
     }
-    public runLinter(document:TextDocument): Promise<baseLinter.ILintMessage[]> {
+    public runLinter(document: TextDocument, cancellation: CancellationToken): Promise<baseLinter.ILintMessage[]> {
         if (!this.pythonSettings.linting.pydocstyleEnabled) {
             return Promise.resolve([]);
         }
 
         let pydocstylePath = this.pythonSettings.linting.pydocstylePath;
         let pydocstyleArgs = Array.isArray(this.pythonSettings.linting.pydocstyleArgs) ? this.pythonSettings.linting.pydocstyleArgs : [];
         return new Promise<baseLinter.ILintMessage[]>(resolve => {
-            this.run(pydocstylePath, pydocstyleArgs.concat([document.uri.fsPath]), document).then(messages => {
+            this.run(pydocstylePath, pydocstyleArgs.concat([document.uri.fsPath]), document, null, cancellation).then(messages => {
                 // All messages in pep8 are treated as warnings for now
                 messages.forEach(msg => {
                     msg.severity = baseLinter.LintMessageSeverity.Information;
@@ -35,11 +35,11 @@ export class Linter extends baseLinter.BaseLinter {
         });
     }
 
-    protected run(commandLine: string, args: string[], document:TextDocument): Promise<ILintMessage[]> {
+    protected run(commandLine: string, args: string[], document: TextDocument, cwd: any, cancellation: CancellationToken): Promise<ILintMessage[]> {
         let outputChannel = this.outputChannel;
 
         return new Promise<ILintMessage[]>((resolve, reject) => {
-            execPythonFile(commandLine, args, this.workspaceRootPath, true).then(data => {
+            execPythonFile(commandLine, args, this.workspaceRootPath, true, null, cancellation).then(data => {
                 outputChannel.append('#'.repeat(10) + 'Linting Output - ' + this.Id + '#'.repeat(10) + '\n');
                 outputChannel.append(data);
                 let outputLines = data.split(/\r?\n/g);
diff --git a/src/client/linters/pylama.ts b/src/client/linters/pylama.ts
@@ -1,9 +1,9 @@
 'use strict';
 
 import * as baseLinter from './baseLinter';
-import {OutputChannel} from 'vscode';
+import { OutputChannel } from 'vscode';
 import { Product } from '../common/installer';
-import { TextDocument } from 'vscode';
+import { TextDocument, CancellationToken } from 'vscode';
 
 const REGEX = '(?<file>.py):(?<line>\\d+):(?<column>\\d+): \\[(?<type>\\w+)\\] (?<code>\\w\\d+):? (?<message>.*)\\r?(\\n|$)';
 
@@ -15,15 +15,15 @@ export class Linter extends baseLinter.BaseLinter {
     public isEnabled(): Boolean {
         return this.pythonSettings.linting.pylamaEnabled;
     }
-    public runLinter(document:TextDocument): Promise<baseLinter.ILintMessage[]> {
+    public runLinter(document: TextDocument, cancellation: CancellationToken): Promise<baseLinter.ILintMessage[]> {
         if (!this.pythonSettings.linting.pylamaEnabled) {
             return Promise.resolve([]);
         }
 
         let pylamaPath = this.pythonSettings.linting.pylamaPath;
         let pylamaArgs = Array.isArray(this.pythonSettings.linting.pylamaArgs) ? this.pythonSettings.linting.pylamaArgs : [];
         return new Promise<baseLinter.ILintMessage[]>(resolve => {
-            this.run(pylamaPath, pylamaArgs.concat(['--format=parsable', document.uri.fsPath]), document, this.workspaceRootPath, REGEX).then(messages => {
+            this.run(pylamaPath, pylamaArgs.concat(['--format=parsable', document.uri.fsPath]), document, this.workspaceRootPath, cancellation, REGEX).then(messages => {
                 // All messages in pylama are treated as warnings for now
                 messages.forEach(msg => {
                     msg.severity = baseLinter.LintMessageSeverity.Information;
diff --git a/src/client/linters/pylint.ts b/src/client/linters/pylint.ts
@@ -3,7 +3,7 @@
 import * as baseLinter from './baseLinter';
 import { OutputChannel } from 'vscode';
 import { Product } from '../common/installer';
-import { TextDocument } from 'vscode';
+import { TextDocument, CancellationToken } from 'vscode';
 
 export class Linter extends baseLinter.BaseLinter {
     constructor(outputChannel: OutputChannel, workspaceRootPath?: string) {
@@ -36,15 +36,15 @@ export class Linter extends baseLinter.BaseLinter {
     public isEnabled(): Boolean {
         return this.pythonSettings.linting.pylintEnabled;
     }
-    public runLinter(document: TextDocument): Promise<baseLinter.ILintMessage[]> {
+    public runLinter(document: TextDocument, cancellation: CancellationToken): Promise<baseLinter.ILintMessage[]> {
         if (!this.pythonSettings.linting.pylintEnabled) {
             return Promise.resolve([]);
         }
 
         let pylintPath = this.pythonSettings.linting.pylintPath;
         let pylintArgs = Array.isArray(this.pythonSettings.linting.pylintArgs) ? this.pythonSettings.linting.pylintArgs : [];
         return new Promise<baseLinter.ILintMessage[]>((resolve, reject) => {
-            this.run(pylintPath, pylintArgs.concat(['--msg-template=\'{line},{column},{category},{msg_id}:{msg}\'', '--reports=n', '--output-format=text', document.uri.fsPath]), document, this.workspaceRootPath).then(messages => {
+            this.run(pylintPath, pylintArgs.concat(['--msg-template=\'{line},{column},{category},{msg_id}:{msg}\'', '--reports=n', '--output-format=text', document.uri.fsPath]), document, this.workspaceRootPath, cancellation).then(messages => {
                 messages.forEach(msg => {
                     msg.severity = this.parseMessagesSeverity(msg.type);
                 });
diff --git a/src/client/providers/lintProvider.ts b/src/client/providers/lintProvider.ts
@@ -162,7 +162,7 @@ export class LintProvider extends vscode.Disposable {
             }
             // turn off telemetry for linters (at least for now)
             //let delays = new telemetryHelper.Delays();
-            return linter.runLinter(document).then(results => {
+            return linter.runLinter(document, cancelToken.token).then(results => {
                 //delays.stop();
                 //telemetryHelper.sendTelemetryEvent(telemetryContracts.IDE.Lint, { Lint_Provider: linter.Id }, delays.toMeasures());
                 return results;
diff --git a/src/test/extension.lint.test.ts b/src/test/extension.lint.test.ts
@@ -190,9 +190,10 @@ suite('Linting', () => {
     });
 
     function testLinterMessages(linter: baseLinter.BaseLinter, outputChannel: MockOutputChannel, pythonFile: string, messagesToBeReceived: baseLinter.ILintMessage[]): Thenable<any> {
+        let cancelToken = new vscode.CancellationTokenSource();
         return vscode.workspace.openTextDocument(pythonFile)
             .then(document => vscode.window.showTextDocument(document))
-            .then(editor => linter.runLinter(editor.document))
+            .then(editor => linter.runLinter(editor.document, cancelToken.token))
             .then(messages => {
                 // Different versions of python return different errors,
                 if (messagesToBeReceived.length === 0) {

Original file line number	Diff line number	Diff line change
`@@ -101,7 +101,7 @@ export function execPythonFile(file: string, args: string[], cwd: string, includ`
`101`	`101`	`if (stdOut) {`
`102`	`102`	`return spawnFileInternal(file, args, { cwd }, includeErrorAsResponse, stdOut, token);`
`103`	`103`	`}`
`104`		`- return execFileInternal(file, args, { cwd: cwd }, includeErrorAsResponse);`
	`104`	`+ return execFileInternal(file, args, { cwd: cwd }, includeErrorAsResponse, token);`
`105`	`105`	`}`
`106`	`106`
`107`	`107`	`return getPythonInterpreterDirectory().then(pyPath => {`
`@@ -110,7 +110,7 @@ export function execPythonFile(file: string, args: string[], cwd: string, includ`
`110`	`110`	`if (stdOut) {`
`111`	`111`	`return spawnFileInternal(file, args, { cwd }, includeErrorAsResponse, stdOut, token);`
`112`	`112`	`}`
`113`		`- return execFileInternal(file, args, { cwd: cwd }, includeErrorAsResponse);`
	`113`	`+ return execFileInternal(file, args, { cwd: cwd }, includeErrorAsResponse, token);`
`114`	`114`	`}`
`115`	`115`
`116`	`116`	`if (customEnvVariables === null) {`
`@@ -134,7 +134,7 @@ export function execPythonFile(file: string, args: string[], cwd: string, includ`
`134`	`134`	`if (stdOut) {`
`135`	`135`	`return spawnFileInternal(file, args, { cwd, env: customEnvVariables }, includeErrorAsResponse, stdOut, token);`
`136`	`136`	`}`
`137`		`- return execFileInternal(file, args, { cwd, env: customEnvVariables }, includeErrorAsResponse);`
	`137`	`+ return execFileInternal(file, args, { cwd, env: customEnvVariables }, includeErrorAsResponse, token);`
`138`	`138`	`});`
`139`	`139`	`}`
`140`	`140`
`@@ -160,11 +160,19 @@ function handleResponse(file: string, includeErrorAsResponse: boolean, error: Er`
`160`	`160`	`resolve(stdout + '');`
`161`	`161`	`});`
`162`	`162`	`}`
`163`		`-function execFileInternal(file: string, args: string[], options: child_process.ExecFileOptions, includeErrorAsResponse: boolean): Promise<string> {`
	`163`	`+function execFileInternal(file: string, args: string[], options: child_process.ExecFileOptions, includeErrorAsResponse: boolean, token?: CancellationToken): Promise<string> {`
`164`	`164`	`return new Promise<string>((resolve, reject) => {`
`165`		`- child_process.execFile(file, args, options, (error, stdout, stderr) => {`
	`165`	`+ let proc = child_process.execFile(file, args, options, (error, stdout, stderr) => {`
`166`	`166`	`handleResponse(file, includeErrorAsResponse, error, stdout, stderr).then(resolve, reject);`
`167`	`167`	`});`
	`168`	`+ if (token && token.onCancellationRequested) {`
	`169`	`+ token.onCancellationRequested(() => {`
	`170`	`+ if (proc) {`
	`171`	`+ proc.kill();`
	`172`	`+ proc = null;`
	`173`	`+ }`
	`174`	`+ });`
	`175`	`+ }`
`168`	`176`	`});`
`169`	`177`	`}`
`170`	`178`	`function spawnFileInternal(file: string, args: string[], options: child_process.ExecFileOptions, includeErrorAsResponse: boolean, stdOut: (line: string) => void, token?: CancellationToken): Promise<string> {`
Original file line number	Diff line number	Diff line change
`@@ -162,7 +162,7 @@ export class LintProvider extends vscode.Disposable {`
`162`	`162`	`}`
`163`	`163`	`// turn off telemetry for linters (at least for now)`
`164`	`164`	`//let delays = new telemetryHelper.Delays();`
`165`		`- return linter.runLinter(document).then(results => {`
	`165`	`+ return linter.runLinter(document, cancelToken.token).then(results => {`
`166`	`166`	`//delays.stop();`
`167`	`167`	`//telemetryHelper.sendTelemetryEvent(telemetryContracts.IDE.Lint, { Lint_Provider: linter.Id }, delays.toMeasures());`
`168`	`168`	`return results;`