From 8a11f88b4562f7f21366ab0af9fc0b8dffdd8e29 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Tue, 6 Aug 2024 16:03:16 -0400 Subject: [PATCH 01/10] Only `await` on the last code block This ensures that all of the blocks are sent over in a single sequential batch, with no way for anyone else to execute their own block between two of ours, which would cause ordering issues --- apps/vscode/src/host/hooks.ts | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/apps/vscode/src/host/hooks.ts b/apps/vscode/src/host/hooks.ts index 6750cc82..b160cde1 100644 --- a/apps/vscode/src/host/hooks.ts +++ b/apps/vscode/src/host/hooks.ts @@ -56,14 +56,32 @@ export function hooksExtensionHost() : ExtensionHost { case "r": return { execute: async (blocks: string[], _editorUri?: vscode.Uri) : Promise => { - for (const block of blocks) { - let code = block; + const runtime = hooksApi()?.runtime; + + const executeCode = async (code: string) => { if (language === "python" && isKnitrDocument(document, engine)) { language = "r"; - code = pythonWithReticulate(block); + code = pythonWithReticulate(code); } - await hooksApi()?.runtime.executeCode(language, code, false); - } + + runtime?.executeCode(language, code, false); + }; + + // Get the last block that we will actually await on. + // If `blocks` is empty, then `lastBlock` is `undefined`. + const lastBlock = blocks.pop(); + + // Execute synchronously to ensure that the blocks are executed in order, + // as opposed to `await`ing at each iteration within the loop. This prevents + // someone else from executing their own block while we `await`, which would + // lead to ordering issues (https://github.com/posit-dev/positron/issues/4231). + for (const block of blocks) { + executeCode(block); + } + + if (lastBlock !== undefined) { + await executeCode(lastBlock); + } }, executeSelection: async () : Promise => { await vscode.commands.executeCommand('workbench.action.positronConsole.executeCode', {languageId: language}); @@ -166,4 +184,3 @@ async function getStatementRange( position ); } - From 5ecff2424d8dc0fc9bd5babe16553a6dc0916db3 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Thu, 5 Sep 2024 11:48:21 -0400 Subject: [PATCH 02/10] Introduce a task queue for running `blocks` --- apps/vscode/src/host/hooks.ts | 151 +++++++++++++++++++++++++++++----- 1 file changed, 130 insertions(+), 21 deletions(-) diff --git a/apps/vscode/src/host/hooks.ts b/apps/vscode/src/host/hooks.ts index b160cde1..b673fc8f 100644 --- a/apps/vscode/src/host/hooks.ts +++ b/apps/vscode/src/host/hooks.ts @@ -20,6 +20,7 @@ import { ExtensionHost, HostWebviewPanel, HostStatementRangeProvider } from '.'; import { CellExecutor, cellExecutorForLanguage, executableLanguages, isKnitrDocument, pythonWithReticulate } from './executors'; import { MarkdownEngine } from '../markdown/engine'; import { virtualDoc, virtualDocUri, adjustedPosition } from "../vdoc/vdoc"; +import { Disposable } from 'vscode'; declare global { function acquirePositronApi() : hooks.PositronApi; @@ -42,6 +43,109 @@ export function hasHooks() { return !!hooksApi(); } +type TaskId = number; + +interface Task { + id: TaskId, + runtime: hooks.PositronRuntime, + language: string, + blocks: string[] +} + +class TaskQueue implements Disposable { + /// Singleton instance + private static _instance: TaskQueue; + + private _id: TaskId = 0; + private _tasks: Task[] = []; + private _running = false; + + private readonly _onDidFinishTask = new vscode.EventEmitter(); + onDidFinishTask = this._onDidFinishTask.event; + + /** + * Disposal method + * + * Not currently used since the singleton is effectively a global variable. + */ + dispose(): void { + this._onDidFinishTask.dispose(); + } + + /** + * Constructor + * + * Private since we only want one of these. Access using `instance()` instead. + */ + private constructor() { } + + /** + * Accessor for the singleton instance + * + * Creates it if it doesn't exist. + */ + static get instance(): TaskQueue { + if (!TaskQueue._instance) { + TaskQueue._instance = new TaskQueue(); + } + return TaskQueue._instance; + } + + /** + * Retrives an `id` to be used with the next task + */ + id(): TaskId { + let id = this._id; + this._id++; + return id; + } + + /** + * Pushes a `task` into the queue. Immediately runs it if nothing else is running. + */ + async push(task: Task) { + this._tasks.push(task); + + // Immediately run the task if possible + this.run(); + } + + /** + * Runs a task in the queue + * + * If we are currently running something else, bails. `run()` will be called again + * once the other task finishes. + */ + private async run() { + if (this._running) { + // Someone else is running, we will get recalled once they finish + return; + } + + const task = this._tasks.pop(); + + if (task === undefined) { + // Nothing to run right now + return; + } + + this._running = true; + + try { + // Run each block sequentially + for (const block of task.blocks) { + await task.runtime.executeCode(task.language, block, false); + } + } finally { + this._running = false; + this._onDidFinishTask.fire(task.id); + } + + // Run next task if one is in the queue + this.run(); + } +} + export function hooksExtensionHost() : ExtensionHost { return { // supported executable languages (we delegate to the default for langugaes @@ -58,30 +162,35 @@ export function hooksExtensionHost() : ExtensionHost { execute: async (blocks: string[], _editorUri?: vscode.Uri) : Promise => { const runtime = hooksApi()?.runtime; - const executeCode = async (code: string) => { - if (language === "python" && isKnitrDocument(document, engine)) { - language = "r"; - code = pythonWithReticulate(code); - } - - runtime?.executeCode(language, code, false); - }; - - // Get the last block that we will actually await on. - // If `blocks` is empty, then `lastBlock` is `undefined`. - const lastBlock = blocks.pop(); - - // Execute synchronously to ensure that the blocks are executed in order, - // as opposed to `await`ing at each iteration within the loop. This prevents - // someone else from executing their own block while we `await`, which would - // lead to ordering issues (https://github.com/posit-dev/positron/issues/4231). - for (const block of blocks) { - executeCode(block); + if (runtime === undefined) { + // Can't do anything without a runtime + return; } - if (lastBlock !== undefined) { - await executeCode(lastBlock); + if (language === "python" && isKnitrDocument(document, engine)) { + language = "r"; + blocks = blocks.map(pythonWithReticulate); } + + // Get our task id and construct the task + const id = TaskQueue.instance.id(); + const task = { id, runtime, language, blocks }; + + // Construct a promise that resolves when our task finishes + const finished = new Promise((resolve, _reject) => { + const handle = TaskQueue.instance.onDidFinishTask((finishedId) => { + if (id === finishedId) { + handle.dispose(); + resolve(); + } + }); + }); + + // Push the task, which may immediately run it, and then wait for it to finish. + // Using a task queue ensures that another call to `execute()` can't interleave + // its own executions while we `await` between `blocks`. + await TaskQueue.instance.push(task); + await finished; }, executeSelection: async () : Promise => { await vscode.commands.executeCommand('workbench.action.positronConsole.executeCode', {languageId: language}); From 407cb664ca98b6c4cbf842f6d95aeeb63a06e51b Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Thu, 5 Sep 2024 13:07:06 -0400 Subject: [PATCH 03/10] Abstract out a `callback` --- apps/vscode/src/host/hooks.ts | 36 ++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/apps/vscode/src/host/hooks.ts b/apps/vscode/src/host/hooks.ts index b673fc8f..06e9a96b 100644 --- a/apps/vscode/src/host/hooks.ts +++ b/apps/vscode/src/host/hooks.ts @@ -44,12 +44,11 @@ export function hasHooks() { } type TaskId = number; +type TaskCallback = () => Promise; interface Task { id: TaskId, - runtime: hooks.PositronRuntime, - language: string, - blocks: string[] + callback: TaskCallback } class TaskQueue implements Disposable { @@ -91,10 +90,18 @@ class TaskQueue implements Disposable { return TaskQueue._instance; } + /** + * Construct a new `Task` that can be pushed onto the queue + */ + task(callback: TaskCallback): Task { + const id = this.id(); + return { id, callback } + } + /** * Retrives an `id` to be used with the next task */ - id(): TaskId { + private id(): TaskId { let id = this._id; this._id++; return id; @@ -132,10 +139,7 @@ class TaskQueue implements Disposable { this._running = true; try { - // Run each block sequentially - for (const block of task.blocks) { - await task.runtime.executeCode(task.language, block, false); - } + await task.callback(); } finally { this._running = false; this._onDidFinishTask.fire(task.id); @@ -171,15 +175,21 @@ export function hooksExtensionHost() : ExtensionHost { language = "r"; blocks = blocks.map(pythonWithReticulate); } + + // Our callback executes each block sequentially + const callback = async () => { + for (const block of blocks) { + await runtime.executeCode(language, block, false); + } + } - // Get our task id and construct the task - const id = TaskQueue.instance.id(); - const task = { id, runtime, language, blocks }; + // Construct a new task that calls our `callback` when its our turn + const task = TaskQueue.instance.task(callback); // Construct a promise that resolves when our task finishes const finished = new Promise((resolve, _reject) => { - const handle = TaskQueue.instance.onDidFinishTask((finishedId) => { - if (id === finishedId) { + const handle = TaskQueue.instance.onDidFinishTask((id) => { + if (task.id === id) { handle.dispose(); resolve(); } From 826cb1da58889f106dddea4f9ad53619d496e866 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Thu, 5 Sep 2024 13:12:51 -0400 Subject: [PATCH 04/10] Move the queue to its own file --- apps/vscode/src/host/hooks.ts | 109 +----------------------------- apps/vscode/src/host/queue.ts | 123 ++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 108 deletions(-) create mode 100644 apps/vscode/src/host/queue.ts diff --git a/apps/vscode/src/host/hooks.ts b/apps/vscode/src/host/hooks.ts index 06e9a96b..74b08ea6 100644 --- a/apps/vscode/src/host/hooks.ts +++ b/apps/vscode/src/host/hooks.ts @@ -20,7 +20,7 @@ import { ExtensionHost, HostWebviewPanel, HostStatementRangeProvider } from '.'; import { CellExecutor, cellExecutorForLanguage, executableLanguages, isKnitrDocument, pythonWithReticulate } from './executors'; import { MarkdownEngine } from '../markdown/engine'; import { virtualDoc, virtualDocUri, adjustedPosition } from "../vdoc/vdoc"; -import { Disposable } from 'vscode'; +import { TaskQueue } from './queue'; declare global { function acquirePositronApi() : hooks.PositronApi; @@ -43,113 +43,6 @@ export function hasHooks() { return !!hooksApi(); } -type TaskId = number; -type TaskCallback = () => Promise; - -interface Task { - id: TaskId, - callback: TaskCallback -} - -class TaskQueue implements Disposable { - /// Singleton instance - private static _instance: TaskQueue; - - private _id: TaskId = 0; - private _tasks: Task[] = []; - private _running = false; - - private readonly _onDidFinishTask = new vscode.EventEmitter(); - onDidFinishTask = this._onDidFinishTask.event; - - /** - * Disposal method - * - * Not currently used since the singleton is effectively a global variable. - */ - dispose(): void { - this._onDidFinishTask.dispose(); - } - - /** - * Constructor - * - * Private since we only want one of these. Access using `instance()` instead. - */ - private constructor() { } - - /** - * Accessor for the singleton instance - * - * Creates it if it doesn't exist. - */ - static get instance(): TaskQueue { - if (!TaskQueue._instance) { - TaskQueue._instance = new TaskQueue(); - } - return TaskQueue._instance; - } - - /** - * Construct a new `Task` that can be pushed onto the queue - */ - task(callback: TaskCallback): Task { - const id = this.id(); - return { id, callback } - } - - /** - * Retrives an `id` to be used with the next task - */ - private id(): TaskId { - let id = this._id; - this._id++; - return id; - } - - /** - * Pushes a `task` into the queue. Immediately runs it if nothing else is running. - */ - async push(task: Task) { - this._tasks.push(task); - - // Immediately run the task if possible - this.run(); - } - - /** - * Runs a task in the queue - * - * If we are currently running something else, bails. `run()` will be called again - * once the other task finishes. - */ - private async run() { - if (this._running) { - // Someone else is running, we will get recalled once they finish - return; - } - - const task = this._tasks.pop(); - - if (task === undefined) { - // Nothing to run right now - return; - } - - this._running = true; - - try { - await task.callback(); - } finally { - this._running = false; - this._onDidFinishTask.fire(task.id); - } - - // Run next task if one is in the queue - this.run(); - } -} - export function hooksExtensionHost() : ExtensionHost { return { // supported executable languages (we delegate to the default for langugaes diff --git a/apps/vscode/src/host/queue.ts b/apps/vscode/src/host/queue.ts new file mode 100644 index 00000000..a623565f --- /dev/null +++ b/apps/vscode/src/host/queue.ts @@ -0,0 +1,123 @@ +/* + * queue.ts + * + * Copyright (C) 2024 by Posit Software, PBC + * + * Unless you have received this program directly from Posit Software pursuant + * to the terms of a commercial license agreement with Posit Software, then + * this program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +import { Disposable, EventEmitter } from 'vscode'; + +export type TaskId = number; +export type TaskCallback = () => Promise; + +export interface Task { + id: TaskId, + callback: TaskCallback +} + +export class TaskQueue implements Disposable { + /// Singleton instance + private static _instance: TaskQueue; + + private _id: TaskId = 0; + private _tasks: Task[] = []; + private _running = false; + + private readonly _onDidFinishTask = new EventEmitter(); + onDidFinishTask = this._onDidFinishTask.event; + + /** + * Disposal method + * + * Not currently used since the singleton is effectively a global variable. + */ + dispose(): void { + this._onDidFinishTask.dispose(); + } + + /** + * Constructor + * + * Private since we only want one of these. Access using `instance()` instead. + */ + private constructor() { } + + /** + * Accessor for the singleton instance + * + * Creates it if it doesn't exist. + */ + static get instance(): TaskQueue { + if (!TaskQueue._instance) { + TaskQueue._instance = new TaskQueue(); + } + return TaskQueue._instance; + } + + /** + * Construct a new `Task` that can be pushed onto the queue + */ + task(callback: TaskCallback): Task { + const id = this.id(); + return { id, callback } + } + + /** + * Retrives an `id` to be used with the next task + */ + private id(): TaskId { + let id = this._id; + this._id++; + return id; + } + + /** + * Pushes a `task` into the queue. Immediately runs it if nothing else is running. + */ + async push(task: Task) { + this._tasks.push(task); + + // Immediately run the task if possible + this.run(); + } + + /** + * Runs a task in the queue + * + * If we are currently running something else, bails. `run()` will be called again + * once the other task finishes. + */ + private async run() { + if (this._running) { + // Someone else is running, we will get recalled once they finish + return; + } + + const task = this._tasks.pop(); + + if (task === undefined) { + // Nothing to run right now + return; + } + + this._running = true; + + try { + await task.callback(); + } finally { + this._running = false; + this._onDidFinishTask.fire(task.id); + } + + // Run next task if one is in the queue + this.run(); + } +} From 285bcfaa5d1b5e46b09fa9c5bc6b3bde52355de5 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Thu, 5 Sep 2024 14:34:51 -0400 Subject: [PATCH 05/10] Simplify API and add ability to have multiple queues --- apps/vscode/src/host/hooks.ts | 21 +------- apps/vscode/src/host/manager.ts | 77 ++++++++++++++++++++++++++ apps/vscode/src/host/queue.ts | 95 ++++++++++++++++----------------- 3 files changed, 124 insertions(+), 69 deletions(-) create mode 100644 apps/vscode/src/host/manager.ts diff --git a/apps/vscode/src/host/hooks.ts b/apps/vscode/src/host/hooks.ts index 74b08ea6..c31a5643 100644 --- a/apps/vscode/src/host/hooks.ts +++ b/apps/vscode/src/host/hooks.ts @@ -20,7 +20,7 @@ import { ExtensionHost, HostWebviewPanel, HostStatementRangeProvider } from '.'; import { CellExecutor, cellExecutorForLanguage, executableLanguages, isKnitrDocument, pythonWithReticulate } from './executors'; import { MarkdownEngine } from '../markdown/engine'; import { virtualDoc, virtualDocUri, adjustedPosition } from "../vdoc/vdoc"; -import { TaskQueue } from './queue'; +import { TaskQueueManager } from './manager'; declare global { function acquirePositronApi() : hooks.PositronApi; @@ -76,24 +76,7 @@ export function hooksExtensionHost() : ExtensionHost { } } - // Construct a new task that calls our `callback` when its our turn - const task = TaskQueue.instance.task(callback); - - // Construct a promise that resolves when our task finishes - const finished = new Promise((resolve, _reject) => { - const handle = TaskQueue.instance.onDidFinishTask((id) => { - if (task.id === id) { - handle.dispose(); - resolve(); - } - }); - }); - - // Push the task, which may immediately run it, and then wait for it to finish. - // Using a task queue ensures that another call to `execute()` can't interleave - // its own executions while we `await` between `blocks`. - await TaskQueue.instance.push(task); - await finished; + await TaskQueueManager.instance.enqueue(language, callback); }, executeSelection: async () : Promise => { await vscode.commands.executeCommand('workbench.action.positronConsole.executeCode', {languageId: language}); diff --git a/apps/vscode/src/host/manager.ts b/apps/vscode/src/host/manager.ts new file mode 100644 index 00000000..8950f47e --- /dev/null +++ b/apps/vscode/src/host/manager.ts @@ -0,0 +1,77 @@ +/* + * manager.ts + * + * Copyright (C) 2024 by Posit Software, PBC + * + * Unless you have received this program directly from Posit Software pursuant + * to the terms of a commercial license agreement with Posit Software, then + * this program is licensed to you under the terms of version 3 of the + * GNU Affero General Public License. This program is distributed WITHOUT + * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the + * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. + * + */ + +import { Disposable } from 'vscode'; +import { TaskCallback, TaskQueue } from './queue'; + +/** + * Manager of multiple task queues + * + * A singleton class that constructs and manages multiple `TaskQueue`s, identified by their `key`. + */ +export class TaskQueueManager implements Disposable { + /// Singleton instance + private static _instance: TaskQueueManager; + + /// Maps a `key` to its `queue` + private _queues = new Map(); + + /** + * Constructor + * + * Private since we only want one of these per `key`. Access using `instance()` instead. + */ + private constructor() { } + + /** + * Accessor for the singleton instance + * + * Creates it if it doesn't exist. + */ + static get instance(): TaskQueueManager { + if (!TaskQueueManager._instance) { + // Initialize manager if we've never accessed it + TaskQueueManager._instance = new TaskQueueManager(); + } + + return TaskQueueManager._instance; + } + + /** + * Enqueue a `callback` for execution on `key`'s task queue + * + * Returns a promise that resolves when the task finishes + */ + async enqueue(key: string, callback: TaskCallback): Promise { + let queue = this._queues.get(key); + + if (queue === undefined) { + // If we've never initialized this key's queue, do so now + queue = new TaskQueue(); + this._queues.set(key, queue); + } + + return queue.enqueue(callback); + } + + /** + * Disposal method + * + * Never called in practice, because this is a singleton and effectively a global. + */ + dispose(): void { + this._queues.forEach((queue) => queue.dispose()); + } +} diff --git a/apps/vscode/src/host/queue.ts b/apps/vscode/src/host/queue.ts index a623565f..d8e7c974 100644 --- a/apps/vscode/src/host/queue.ts +++ b/apps/vscode/src/host/queue.ts @@ -23,70 +23,46 @@ export interface Task { callback: TaskCallback } +/** + * A task queue that maintains the ordering of tasks submitted to the queue + */ export class TaskQueue implements Disposable { - /// Singleton instance - private static _instance: TaskQueue; - private _id: TaskId = 0; private _tasks: Task[] = []; private _running = false; private readonly _onDidFinishTask = new EventEmitter(); - onDidFinishTask = this._onDidFinishTask.event; + private readonly onDidFinishTask = this._onDidFinishTask.event; - /** - * Disposal method - * - * Not currently used since the singleton is effectively a global variable. - */ dispose(): void { this._onDidFinishTask.dispose(); } /** - * Constructor - * - * Private since we only want one of these. Access using `instance()` instead. + * Enqueue a `callback` for execution + * + * Returns a promise that resolves when the task finishes */ - private constructor() { } - - /** - * Accessor for the singleton instance - * - * Creates it if it doesn't exist. - */ - static get instance(): TaskQueue { - if (!TaskQueue._instance) { - TaskQueue._instance = new TaskQueue(); - } - return TaskQueue._instance; - } - - /** - * Construct a new `Task` that can be pushed onto the queue - */ - task(callback: TaskCallback): Task { - const id = this.id(); - return { id, callback } - } - - /** - * Retrives an `id` to be used with the next task - */ - private id(): TaskId { - let id = this._id; - this._id++; - return id; - } - - /** - * Pushes a `task` into the queue. Immediately runs it if nothing else is running. - */ - async push(task: Task) { + async enqueue(callback: TaskCallback): Promise { + // Create an official task out of this callback + const task = this.task(callback); + + // Create a promise that resolves when the task is done + const out = new Promise((resolve, _reject) => { + const handle = this.onDidFinishTask((id) => { + if (task.id === id) { + handle.dispose(); + resolve(); + } + }); + }); + + // Put the task on the back of the queue. + // Immediately run it if possible. this._tasks.push(task); - - // Immediately run the task if possible this.run(); + + return out; } /** @@ -101,7 +77,8 @@ export class TaskQueue implements Disposable { return; } - const task = this._tasks.pop(); + // Pop off the first task in the queue + const task = this._tasks.shift(); if (task === undefined) { // Nothing to run right now @@ -114,10 +91,28 @@ export class TaskQueue implements Disposable { await task.callback(); } finally { this._running = false; + // Let the promise know this task is done this._onDidFinishTask.fire(task.id); } // Run next task if one is in the queue this.run(); } + + /** + * Construct a new `Task` that can be pushed onto the queue + */ + private task(callback: TaskCallback): Task { + const id = this.id(); + return { id, callback } + } + + /** + * Retrives an `id` to be used with the next task + */ + private id(): TaskId { + let id = this._id; + this._id++; + return id; + } } From 4f624226d8bb217dfdf58511dbf35fee84ff142c Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Thu, 5 Sep 2024 14:47:31 -0400 Subject: [PATCH 06/10] `await` calls to `run()` --- apps/vscode/src/host/queue.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/vscode/src/host/queue.ts b/apps/vscode/src/host/queue.ts index d8e7c974..1f49d7fb 100644 --- a/apps/vscode/src/host/queue.ts +++ b/apps/vscode/src/host/queue.ts @@ -40,7 +40,7 @@ export class TaskQueue implements Disposable { /** * Enqueue a `callback` for execution - * + * * Returns a promise that resolves when the task finishes */ async enqueue(callback: TaskCallback): Promise { @@ -60,7 +60,7 @@ export class TaskQueue implements Disposable { // Put the task on the back of the queue. // Immediately run it if possible. this._tasks.push(task); - this.run(); + await this.run(); return out; } @@ -96,7 +96,7 @@ export class TaskQueue implements Disposable { } // Run next task if one is in the queue - this.run(); + await this.run(); } /** From 8decce42722ca38d9b781383f9d92d0c2d2599e0 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Mon, 9 Sep 2024 10:19:44 -0400 Subject: [PATCH 07/10] Switch to more official PQueue Lmao at the fact that I implemented this from scratch --- apps/vscode/package.json | 1 + apps/vscode/src/host/hooks.ts | 2 +- apps/vscode/src/host/manager.ts | 35 ++++++++++++--------------------- yarn.lock | 18 +++++++++++++++++ 4 files changed, 33 insertions(+), 23 deletions(-) diff --git a/apps/vscode/package.json b/apps/vscode/package.json index 9e7e0455..c7372613 100644 --- a/apps/vscode/package.json +++ b/apps/vscode/package.json @@ -1356,6 +1356,7 @@ "markdown-it": "^13.0.1", "markdown-it-highlightjs": "^4.0.1", "nanoid": "^4.0.0", + "p-queue": "^8.0.1", "picomatch": "^2.3.1", "quarto-core": "*", "quarto-lsp": "*", diff --git a/apps/vscode/src/host/hooks.ts b/apps/vscode/src/host/hooks.ts index c31a5643..dd492504 100644 --- a/apps/vscode/src/host/hooks.ts +++ b/apps/vscode/src/host/hooks.ts @@ -76,7 +76,7 @@ export function hooksExtensionHost() : ExtensionHost { } } - await TaskQueueManager.instance.enqueue(language, callback); + await TaskQueueManager.instance.add(language, callback); }, executeSelection: async () : Promise => { await vscode.commands.executeCommand('workbench.action.positronConsole.executeCode', {languageId: language}); diff --git a/apps/vscode/src/host/manager.ts b/apps/vscode/src/host/manager.ts index 8950f47e..4de671e5 100644 --- a/apps/vscode/src/host/manager.ts +++ b/apps/vscode/src/host/manager.ts @@ -13,20 +13,19 @@ * */ -import { Disposable } from 'vscode'; -import { TaskCallback, TaskQueue } from './queue'; +import PQueue from 'p-queue'; /** - * Manager of multiple task queues - * - * A singleton class that constructs and manages multiple `TaskQueue`s, identified by their `key`. + * Manager of multiple queues + * + * A singleton class that constructs and manages multiple queues, identified by their `key`. */ -export class TaskQueueManager implements Disposable { +export class TaskQueueManager { /// Singleton instance private static _instance: TaskQueueManager; /// Maps a `key` to its `queue` - private _queues = new Map(); + private _queues = new Map(); /** * Constructor @@ -50,28 +49,20 @@ export class TaskQueueManager implements Disposable { } /** - * Enqueue a `callback` for execution on `key`'s task queue - * + * Add a `callback` for execution on `key`'s task queue + * * Returns a promise that resolves when the task finishes */ - async enqueue(key: string, callback: TaskCallback): Promise { + async add(key: string, callback: () => Promise): Promise { let queue = this._queues.get(key); if (queue === undefined) { - // If we've never initialized this key's queue, do so now - queue = new TaskQueue(); + // If we've never initialized this key's queue, do so now. + // Limit `concurrency` to 1, because we don't want tasks run out of order. + queue = new PQueue({ concurrency: 1 }); this._queues.set(key, queue); } - return queue.enqueue(callback); - } - - /** - * Disposal method - * - * Never called in practice, because this is a singleton and effectively a global. - */ - dispose(): void { - this._queues.forEach((queue) => queue.dispose()); + return queue.add(callback); } } diff --git a/yarn.lock b/yarn.lock index 1661c7d7..e84b8253 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4306,6 +4306,11 @@ eventemitter3@^4.0.0: resolved "https://registry.yarnpkg.com/eventemitter3/-/eventemitter3-4.0.7.tgz#2de9b68f6528d5644ef5c59526a1b4a07306169f" integrity sha512-8guHBZCwKnFhYdHr2ysuRWErTwhoN2X8XELRlrRwpmfeY2jjuUN4taQMsULKUVo1K4DvZl+0pgfyoysHxvmvEw== +eventemitter3@^5.0.1: + version "5.0.1" + resolved "https://registry.yarnpkg.com/eventemitter3/-/eventemitter3-5.0.1.tgz#53f5ffd0a492ac800721bb42c66b841de96423c4" + integrity sha512-GWkBvjiSZK87ELrYOSESUYeVIc9mvLLf/nXalMOS5dYrgZq9o5OVkbZAVM06CVxYsCwH9BDZFPlQTlPA1j4ahA== + exceljs@^4.3.0: version "4.3.0" resolved "https://registry.yarnpkg.com/exceljs/-/exceljs-4.3.0.tgz#939bc0d4c59c200acadb7051be34d25c109853c4" @@ -6342,6 +6347,19 @@ p-locate@^5.0.0: dependencies: p-limit "^3.0.2" +p-queue@^8.0.1: + version "8.0.1" + resolved "https://registry.yarnpkg.com/p-queue/-/p-queue-8.0.1.tgz#718b7f83836922ef213ddec263ff4223ce70bef8" + integrity sha512-NXzu9aQJTAzbBqOt2hwsR63ea7yvxJc0PwN/zobNAudYfb1B7R08SzB4TsLeSbUCuG467NhnoT0oO6w1qRO+BA== + dependencies: + eventemitter3 "^5.0.1" + p-timeout "^6.1.2" + +p-timeout@^6.1.2: + version "6.1.2" + resolved "https://registry.yarnpkg.com/p-timeout/-/p-timeout-6.1.2.tgz#22b8d8a78abf5e103030211c5fc6dee1166a6aa5" + integrity sha512-UbD77BuZ9Bc9aABo74gfXhNvzC9Tx7SxtHSh1fxvx3jTLLYvmVhiQZZrJzqqU0jKbN32kb5VOKiLEQI/3bIjgQ== + pako@~1.0.2: version "1.0.11" resolved "https://registry.yarnpkg.com/pako/-/pako-1.0.11.tgz#6c9599d340d54dfd3946380252a35705a6b992bf" From a18dd6d751e189c0efb610c28812346ce676f4a3 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Mon, 9 Sep 2024 10:24:21 -0400 Subject: [PATCH 08/10] Rename to `ExecuteQueue` --- .../src/host/{manager.ts => execute-queue.ts} | 21 ++++++++++--------- apps/vscode/src/host/hooks.ts | 4 ++-- 2 files changed, 13 insertions(+), 12 deletions(-) rename apps/vscode/src/host/{manager.ts => execute-queue.ts} (76%) diff --git a/apps/vscode/src/host/manager.ts b/apps/vscode/src/host/execute-queue.ts similarity index 76% rename from apps/vscode/src/host/manager.ts rename to apps/vscode/src/host/execute-queue.ts index 4de671e5..5096e366 100644 --- a/apps/vscode/src/host/manager.ts +++ b/apps/vscode/src/host/execute-queue.ts @@ -1,5 +1,5 @@ /* - * manager.ts + * execute-queue.ts * * Copyright (C) 2024 by Posit Software, PBC * @@ -16,13 +16,14 @@ import PQueue from 'p-queue'; /** - * Manager of multiple queues + * Language specific execution queue * - * A singleton class that constructs and manages multiple queues, identified by their `key`. + * A singleton class that constructs and manages multiple execution queues, identified by + * their `key` (typically the language name). */ -export class TaskQueueManager { +export class ExecuteQueue { /// Singleton instance - private static _instance: TaskQueueManager; + private static _instance: ExecuteQueue; /// Maps a `key` to its `queue` private _queues = new Map(); @@ -39,13 +40,13 @@ export class TaskQueueManager { * * Creates it if it doesn't exist. */ - static get instance(): TaskQueueManager { - if (!TaskQueueManager._instance) { - // Initialize manager if we've never accessed it - TaskQueueManager._instance = new TaskQueueManager(); + static get instance(): ExecuteQueue { + if (!ExecuteQueue._instance) { + // Initialize if we've never accessed it + ExecuteQueue._instance = new ExecuteQueue(); } - return TaskQueueManager._instance; + return ExecuteQueue._instance; } /** diff --git a/apps/vscode/src/host/hooks.ts b/apps/vscode/src/host/hooks.ts index dd492504..91aa8946 100644 --- a/apps/vscode/src/host/hooks.ts +++ b/apps/vscode/src/host/hooks.ts @@ -18,9 +18,9 @@ import * as hooks from 'positron'; import { ExtensionHost, HostWebviewPanel, HostStatementRangeProvider } from '.'; import { CellExecutor, cellExecutorForLanguage, executableLanguages, isKnitrDocument, pythonWithReticulate } from './executors'; +import { ExecuteQueue } from './execute-queue'; import { MarkdownEngine } from '../markdown/engine'; import { virtualDoc, virtualDocUri, adjustedPosition } from "../vdoc/vdoc"; -import { TaskQueueManager } from './manager'; declare global { function acquirePositronApi() : hooks.PositronApi; @@ -76,7 +76,7 @@ export function hooksExtensionHost() : ExtensionHost { } } - await TaskQueueManager.instance.add(language, callback); + await ExecuteQueue.instance.add(language, callback); }, executeSelection: async () : Promise => { await vscode.commands.executeCommand('workbench.action.positronConsole.executeCode', {languageId: language}); From b3124efc45d74406888d8707cb6421328ad5506f Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Mon, 9 Sep 2024 10:33:29 -0400 Subject: [PATCH 09/10] Tweak docs --- apps/vscode/src/host/execute-queue.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/vscode/src/host/execute-queue.ts b/apps/vscode/src/host/execute-queue.ts index 5096e366..bb76ab5a 100644 --- a/apps/vscode/src/host/execute-queue.ts +++ b/apps/vscode/src/host/execute-queue.ts @@ -31,7 +31,7 @@ export class ExecuteQueue { /** * Constructor * - * Private since we only want one of these per `key`. Access using `instance()` instead. + * Private since we only want one of these. Access using `instance()` instead. */ private constructor() { } From 037e957a87db314e2209a502c75d9a8d4f6f6138 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Mon, 9 Sep 2024 10:34:42 -0400 Subject: [PATCH 10/10] Delete `queue.ts` --- apps/vscode/src/host/queue.ts | 118 ---------------------------------- 1 file changed, 118 deletions(-) delete mode 100644 apps/vscode/src/host/queue.ts diff --git a/apps/vscode/src/host/queue.ts b/apps/vscode/src/host/queue.ts deleted file mode 100644 index 1f49d7fb..00000000 --- a/apps/vscode/src/host/queue.ts +++ /dev/null @@ -1,118 +0,0 @@ -/* - * queue.ts - * - * Copyright (C) 2024 by Posit Software, PBC - * - * Unless you have received this program directly from Posit Software pursuant - * to the terms of a commercial license agreement with Posit Software, then - * this program is licensed to you under the terms of version 3 of the - * GNU Affero General Public License. This program is distributed WITHOUT - * ANY EXPRESS OR IMPLIED WARRANTY, INCLUDING THOSE OF NON-INFRINGEMENT, - * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. Please refer to the - * AGPL (http://www.gnu.org/licenses/agpl-3.0.txt) for more details. - * - */ - -import { Disposable, EventEmitter } from 'vscode'; - -export type TaskId = number; -export type TaskCallback = () => Promise; - -export interface Task { - id: TaskId, - callback: TaskCallback -} - -/** - * A task queue that maintains the ordering of tasks submitted to the queue - */ -export class TaskQueue implements Disposable { - private _id: TaskId = 0; - private _tasks: Task[] = []; - private _running = false; - - private readonly _onDidFinishTask = new EventEmitter(); - private readonly onDidFinishTask = this._onDidFinishTask.event; - - dispose(): void { - this._onDidFinishTask.dispose(); - } - - /** - * Enqueue a `callback` for execution - * - * Returns a promise that resolves when the task finishes - */ - async enqueue(callback: TaskCallback): Promise { - // Create an official task out of this callback - const task = this.task(callback); - - // Create a promise that resolves when the task is done - const out = new Promise((resolve, _reject) => { - const handle = this.onDidFinishTask((id) => { - if (task.id === id) { - handle.dispose(); - resolve(); - } - }); - }); - - // Put the task on the back of the queue. - // Immediately run it if possible. - this._tasks.push(task); - await this.run(); - - return out; - } - - /** - * Runs a task in the queue - * - * If we are currently running something else, bails. `run()` will be called again - * once the other task finishes. - */ - private async run() { - if (this._running) { - // Someone else is running, we will get recalled once they finish - return; - } - - // Pop off the first task in the queue - const task = this._tasks.shift(); - - if (task === undefined) { - // Nothing to run right now - return; - } - - this._running = true; - - try { - await task.callback(); - } finally { - this._running = false; - // Let the promise know this task is done - this._onDidFinishTask.fire(task.id); - } - - // Run next task if one is in the queue - await this.run(); - } - - /** - * Construct a new `Task` that can be pushed onto the queue - */ - private task(callback: TaskCallback): Task { - const id = this.id(); - return { id, callback } - } - - /** - * Retrives an `id` to be used with the next task - */ - private id(): TaskId { - let id = this._id; - this._id++; - return id; - } -}