From 3f28c071af091154c7f87945ed093b5b4c227823 Mon Sep 17 00:00:00 2001 From: Martin Duhem Date: Sat, 1 Dec 2018 14:47:56 +0100 Subject: [PATCH 1/2] Fix race condition in worksheet output Sometimes, the `ApplyEdits` that we use in the worksheet to insert blank lines so that the worksheet output fits were reordered. The lead to some output of the worksheet being lost, with decorations being inserted after the end of the file. We know keep track of the edits that we want to apply to the worksheet and ensure that they are executed in the same order as the messages were received. --- vscode-dotty/src/worksheet.ts | 53 ++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/vscode-dotty/src/worksheet.ts b/vscode-dotty/src/worksheet.ts index a95a854a98f0..91c52042c97e 100644 --- a/vscode-dotty/src/worksheet.ts +++ b/vscode-dotty/src/worksheet.ts @@ -63,6 +63,14 @@ class Worksheet implements Disposable { */ private canceller?: CancellationTokenSource = undefined + /** + * The edits that should be applied to this worksheet. + * + * This is used to ensure that the blank lines added to fit the output of the worksheet + * are inserted in the same order as the output arrived. + */ + private applyEdits: Promise = Promise.resolve() + constructor(readonly document: vscode.TextDocument, readonly client: BaseLanguageClient) { } @@ -73,7 +81,7 @@ class Worksheet implements Disposable { this.canceller = undefined } this._onDidStateChange.dispose() - } + } /** Remove all decorations, and resets this worksheet. */ private reset(): void { @@ -83,6 +91,7 @@ class Worksheet implements Disposable { this.decoratedLines.clear() this.runVersion = -1 this.margin = this.longestLine() + 5 + this.applyEdits = Promise.resolve() } /** @@ -111,6 +120,13 @@ class Worksheet implements Disposable { return this.canceller != undefined } + /** Display the output in the worksheet's editor. */ + handleMessage(output: WorksheetPublishOutputParams, editor: vscode.TextEditor) { + this.applyEdits = this.applyEdits.then(() => { + this.displayAndSaveResult(output.line - 1, output.content, editor) + }) + } + /** * Run the worksheet in `document`, if a previous run is in progress, it is * cancelled first. @@ -160,10 +176,10 @@ class Worksheet implements Disposable { * @param runResult The result itself. * @param worksheet The worksheet that receives the result. * @param editor The editor where to display the result. - * @return A `Thenable` that will insert necessary lines to fit the output + * @return A `Promise` that will insert necessary lines to fit the output * and display the decorations upon completion. */ - public displayAndSaveResult(lineNumber: number, runResult: string, editor: vscode.TextEditor) { + public async displayAndSaveResult(lineNumber: number, runResult: string, editor: vscode.TextEditor): Promise { const resultLines = runResult.trim().split(/\r\n|\r|\n/g) // The line where the next decoration should be put. @@ -183,21 +199,18 @@ class Worksheet implements Disposable { this.runVersion += 1 } - return vscode.workspace.applyEdit(addNewLinesEdit).then(_ => { - for (let line of resultLines) { - const decorationPosition = new vscode.Position(actualLine, 0) - const decorationMargin = this.margin - editor.document.lineAt(actualLine).text.length - const decorationType = this.createDecoration(decorationMargin, line) - const decorationOptions = { range: new vscode.Range(decorationPosition, decorationPosition), hoverMessage: line } - const decoration = new Decoration(decorationType, decorationOptions) - - this.decoratedLines.add(actualLine) - this.decorations.push(decoration) - - editor.setDecorations(decorationType, [decorationOptions]) - actualLine += 1 - } - }) + await vscode.workspace.applyEdit(addNewLinesEdit); + for (let line of resultLines) { + const decorationPosition = new vscode.Position(actualLine, 0); + const decorationMargin = this.margin - editor.document.lineAt(actualLine).text.length; + const decorationType = this.createDecoration(decorationMargin, line); + const decorationOptions = { range: new vscode.Range(decorationPosition, decorationPosition), hoverMessage: line }; + const decoration = new Decoration(decorationType, decorationOptions); + this.decoratedLines.add(actualLine); + this.decorations.push(decoration); + editor.setDecorations(decorationType, [decorationOptions]); + actualLine += 1; + } } /** @@ -404,7 +417,7 @@ export class WorksheetProvider implements Disposable { * Handle the result of running part of a worksheet. * This is called when we receive a `worksheet/publishOutput`. * - * @param message The result of running part of a worksheet. + * @param output The result of running part of a worksheet. */ private handleMessage(output: WorksheetPublishOutputParams) { const editor = vscode.window.visibleTextEditors.find(e => { @@ -415,7 +428,7 @@ export class WorksheetProvider implements Disposable { if (editor) { const worksheet = this.worksheetFor(editor.document) if (worksheet) { - worksheet.displayAndSaveResult(output.line - 1, output.content, editor) + worksheet.handleMessage(output, editor) } } } From 843d595f2983cbbf32dc920cf96adc287673846c Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Mon, 3 Dec 2018 18:46:29 +0100 Subject: [PATCH 2/2] Always run the worksheet after all changes have been applied In 23785e6bd3649ff0d2750da42c67bcbb0c56d71c, we moved worksheet.run() from didSave to willSave since didSave is not called when the document is not dirty, unfortunately this gives the wrong behavior when the document _is_ dirty: it means we're going to send a run worksheet request to the server before the server has seen all the changes to the document, in particular with multi-line output this means that the server will see the output with the added empty lines, which means the line numbers in worksheet/publishOutput will be wrong. To avoid this issue we now run the worksheet either on willSave or didSave depending on whether the document is dirty or not. To reproduce the issue, you can create a worksheet with: ``` val x = """ """ "hi" ``` in it, then save the document multiple times. --- vscode-dotty/src/worksheet.ts | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/vscode-dotty/src/worksheet.ts b/vscode-dotty/src/worksheet.ts index 91c52042c97e..598ec29bd41a 100644 --- a/vscode-dotty/src/worksheet.ts +++ b/vscode-dotty/src/worksheet.ts @@ -23,6 +23,13 @@ export const worksheetRunKey = "dotty.worksheet.run" */ export const worksheetCancelKey = "dotty.worksheet.cancel" +/** + * If true, the setting for running the worksheet on save is enabled. + */ +function runWorksheetOnSave(): boolean { + return vscode.workspace.getConfiguration("dotty").get("runWorksheetOnSave") as boolean +} + /** * A wrapper around the information that VSCode needs to display text decorations. * @@ -338,11 +345,25 @@ export class WorksheetProvider implements Disposable { codeLensProvider, vscode.languages.registerCodeLensProvider(documentSelector, codeLensProvider), vscode.workspace.onWillSaveTextDocument(event => { - const runWorksheetOnSave = vscode.workspace.getConfiguration("dotty").get("runWorksheetOnSave") - const worksheet = this.worksheetFor(event.document) + const document = event.document + const worksheet = this.worksheetFor(document) if (worksheet) { event.waitUntil(Promise.resolve(worksheet.prepareRun())) - if (runWorksheetOnSave) worksheet.run() + // If the document is not dirty, then `onDidSaveTextDocument` will not + // be called so we need to run the worksheet now. + // On the other hand, if the document _is_ dirty, we should _not_ run + // the worksheet now because the server state will not be synchronized + // with the client state, instead we let `onDidSaveTextDocument` + // handle it. + if (runWorksheetOnSave() && !document.isDirty) { + worksheet.run() + } + } + }), + vscode.workspace.onDidSaveTextDocument(document => { + const worksheet = this.worksheetFor(document) + if (worksheet && runWorksheetOnSave()) { + worksheet.run() } }), vscode.workspace.onDidCloseTextDocument(document => {