Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid attempting to process pending input if we are already processing it #4257

Merged
merged 2 commits into from
Aug 8, 2024
+122 −58
Merged

Avoid attempting to process pending input if we are already processing it #4257

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
80f38f1
Avoid attempting to process pending input if we are already processin…
DavisVaughan Aug 6, 2024
d9133f7
Slight tweak, plus add more commentary
DavisVaughan Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
178 changes: 120 additions & 58 deletions src/vs/workbench/services/positronConsole/browser/positronConsoleService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,11 @@ class PositronConsoleInstance extends Disposable implements IPositronConsoleInst
*/
private _runtimeItemPendingInput?: RuntimeItemPendingInput;

/**
* Determines whether or not pending input is currently being processed.
*/
private _pendingInputState: 'Idle' | 'Processing' | 'Interrupted' = 'Idle';

/**
* Gets or sets the trim counter.
*/
Expand Down Expand Up @@ -1897,6 +1902,11 @@ class PositronConsoleInstance extends Disposable implements IPositronConsoleInst

// Clear the pending input runtime item.
this._runtimeItemPendingInput = undefined;

// If `processPendingInput()` is running, let it know that it has been interrupted.
if (this._pendingInputState === 'Processing') {
this._pendingInputState = 'Interrupted';
}
}
}

Expand All @@ -1921,90 +1931,142 @@ class PositronConsoleInstance extends Disposable implements IPositronConsoleInst
* Processes pending input.
*/
private async processPendingInput(): Promise<void> {
// If there isn't a pending input runtime item, return.
if (!this._runtimeItemPendingInput) {
// If we are already processing pending input and the `await` below allowed us to loop
// back into here, refuse to process anything else right now.
if (this._pendingInputState !== 'Idle') {
return;
}

// Get the index of the pending input runtime item.
const index = this.runtimeItems.indexOf(this._runtimeItemPendingInput);
this._pendingInputState = 'Processing';

// This index should always be > -1, but be defensive.
if (index > -1) {
this._runtimeItems.splice(index, 1);
try {
return await this.processPendingInputImpl();
} finally {
this._pendingInputState = 'Idle';
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make this:

try {
	await this.processPendingInputImpl();
} finally {
	this._pendingInputState = 'Idle';
}

or

try {
	return this.processPendingInputImpl();
} finally {
	this._pendingInputState = 'Idle';
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I see it:

// Does not do what we want, i.e. does not return the result of the impl helper
await this.processPendingInputImpl();

// Breaks the async / await abstraction by returning a promise instead of a value
return this.processPendingInputImpl();

// Matches the async /await abstraction and correctly returns the result
return await this.processPendingInputImpl();

}

// Get the pending input lines and clear the pending input runtime item.
const pendingInputLines = this._runtimeItemPendingInput.code.split('\n');
this._runtimeItemPendingInput = undefined;
private async processPendingInputImpl(): Promise<void> {
// If there isn't a pending input runtime item, return.
if (!this._runtimeItemPendingInput) {
return;
}

// Find a complete code fragment to execute.
let code = undefined;
const codeLines: string[] = [];

// Get the pending input lines. We keep this up to date at every iteration so it always
// reflects the current state of the pending input, even after an `await`, which may have
// allowed the user to append more pending input code.
let pendingInputLines = this._runtimeItemPendingInput.code.split('\n');

for (let i = 0; i < pendingInputLines.length; i++) {
// Push the pending input line to the code lines.
codeLines.push(pendingInputLines[i]);

// Determine whether the code lines are a complete code fragment. If they are, execute
// the code fragment.
// Determine whether the code lines are a complete code fragment.
const codeFragment = codeLines.join('\n');
const codeFragmentStatus = await this.session.isCodeFragmentComplete(codeFragment);

// If we have been interrupted, then `clearPendingInput()` has reset
// `_runtimeItemPendingInput` and there is nothing for us to do.
if (this._pendingInputState === 'Interrupted') {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@softwarenerd had a question about whether or not we need the Interrupted state here. His point was that maybe we could just check if this._runtimeItemPendingInput == undefined after each await, and if it was, treat that as the interrupt.

I thought about this a little more and determined we do still need it. In theory it is possible for this to happen during a single await:

  • clearPendingInput() can be called, setting this._runtimeItemPendingInput == undefined
  • addPendingInput() can be called, setting this._runtimeItemPendingInput to a new pending input

In that case, once we get control back we won't be able to tell that we got interrupted if we don't have this 'Interrupted' state that gets fired from clearPendingInput()

return;
}

// SAFETY: We expect that `this._runtimeItemPendingInput.code` will only ever grow.
// Also, the update of `pendingInputLines` must happen before we break, because we use
// it below.
pendingInputLines = this._runtimeItemPendingInput.code.split('\n');

if (codeFragmentStatus === RuntimeCodeFragmentStatus.Complete) {
// Create the ID for the code fragment that will be executed.
const id = `fragment-${generateUuid()}`;
code = codeFragment;
break;
}
}

// Add the provisional ActivityItemInput for the code fragment.
const runtimeItemActivity = new RuntimeItemActivity(
id,
new ActivityItemInput(
ActivityItemInputState.Provisional,
id,
id,
new Date(),
this._session.dynState.inputPrompt,
this._session.dynState.continuationPrompt,
codeFragment
)
);
this._runtimeItems.push(runtimeItemActivity);
this._runtimeItemActivities.set(id, runtimeItemActivity);

// If there are remaining pending input lines, add them in a new pending input
// runtime item so they are processed the next time the runtime becomes idle.
if (i + 1 < pendingInputLines.length) {
// Create the pending input runtime item.
this._runtimeItemPendingInput = new RuntimeItemPendingInput(
generateUuid(),
this._session.dynState.inputPrompt,
pendingInputLines.slice(i + 1).join('\n')
);
// Get the index of the pending input runtime item.
const index = this.runtimeItems.indexOf(this._runtimeItemPendingInput);

// Add the pending input runtime item.
this._runtimeItems.push(this._runtimeItemPendingInput);
}
// Remove the current pending input runtime item. We are either done with it entirely, or
// we are going to update it with a new one if we have remaining pending lines.
// This index should always be > -1, but be defensive.
if (index > -1) {
this._runtimeItems.splice(index, 1);
}

// Fire the runtime items changed event once, now, after everything is set up.
this._onDidChangeRuntimeItemsEmitter.fire();
// If we didn't find a complete fragment, set the pending code to everything in the
// (possibly updated) pending input item and let the pending code path handle it.
if (code === undefined) {
// We removed the pending input runtime item, so emit the change.
this._onDidChangeRuntimeItemsEmitter.fire();

// Execute the code fragment.
this.session.execute(
codeFragment,
id,
RuntimeCodeExecutionMode.Interactive,
RuntimeErrorBehavior.Continue);
// The pending input line(s) now become the pending code.
// This fires an event allowing the `ConsoleInput` to update its code editor widget,
// allowing the user to keep typing to eventually generate a complete code chunk.
this.setPendingCode(this._runtimeItemPendingInput.code);

// Fire the onDidExecuteCode event.
this._onDidExecuteCodeEmitter.fire(codeFragment);
// And we no longer have a pending input item.
this._runtimeItemPendingInput = undefined;

// Return.
return;
}
return;
}

// Create the ID for the code fragment that will be executed.
const id = `fragment-${generateUuid()}`;

// Add the provisional ActivityItemInput for the code fragment.
const runtimeItemActivity = new RuntimeItemActivity(
id,
new ActivityItemInput(
ActivityItemInputState.Provisional,
id,
id,
new Date(),
this._session.dynState.inputPrompt,
this._session.dynState.continuationPrompt,
code
)
);
this._runtimeItems.push(runtimeItemActivity);
this._runtimeItemActivities.set(id, runtimeItemActivity);

// If there are remaining pending input lines, add them in a new pending input
// runtime item so they are processed the next time the runtime becomes idle.
const nCodeLines = codeLines.length;
const nPendingLines = pendingInputLines.length;

if (nCodeLines < nPendingLines) {
// Create the new pending input runtime item.
this._runtimeItemPendingInput = new RuntimeItemPendingInput(
generateUuid(),
this._session.dynState.inputPrompt,
pendingInputLines.slice(nCodeLines).join('\n')
);

// Add the pending input runtime item.
this._runtimeItems.push(this._runtimeItemPendingInput);
} else if (nCodeLines == nPendingLines) {
// We are about to execute everything available, so there isn't a new pending input item.
this._runtimeItemPendingInput = undefined;
} else {
throw new Error('Unexpected state. Can\'t have more code lines than pending lines.');
}

// Fire the onDidChangeRuntimeItems event because we removed the pending input runtime item.
// Fire the runtime items changed event once, now, after everything is set up.
this._onDidChangeRuntimeItemsEmitter.fire();

// The pending input line(s) now become the pending code.
this.setPendingCode(pendingInputLines.join('\n'));
// Execute the code fragment.
this.session.execute(
code,
id,
RuntimeCodeExecutionMode.Interactive,
RuntimeErrorBehavior.Continue
);

// Fire the onDidExecuteCode event.
this._onDidExecuteCodeEmitter.fire(code);
}

/**
Expand Down
Loading