Skip to content

Commit

Permalink
Avoid attempting to process pending input if we are already processin…
Browse files Browse the repository at this point in the history 
…g it (#4257)

Addresses #4068

Joint work with @lionel- 


#4068 (comment)
is a detailed account of the problem that this PR addresses, so
definitely give that a close read before trying to understand this.

The reproducible example that is now fixed is
#4068 (comment)

The main challenge to deal with is the fact that `await
this.session.isCodeFragmentComplete(codeFragment)` is async and is
called in a loop inside `processPendingInput()`, allowing the "state of
the world", particularly `this._runtimeItemPendingInput`, to change out
from under us while we are finding a complete code fragment.

There are a few key changes in this PR
- We introduce a global `_pendingInputState` to ensure that we cannot
re-enter `processPendingInput()` while we are already processing input
    - If `'Idle'`, we aren't processing any input
- If `'Processing'`, we are processing input and cannot call
`processPendingInput()` again
- If `'Interrupted'`, this is a signal that can happen while we are
`'Processing'` inside `processPendingInput()` that we should bail out
and stop trying to processing any pending input.

- We no longer set `this._runtimeItemPendingInput = undefined;` in
`processPendingInput()` before all of the `await
isCodeFragmentComplete()` calls. This was particularly bad because at
the `await` point we could loop back around into `addPendingInput()` and
create an entirely new `this._runtimeItemPendingInput` because it looked
like one didn't exist at all!

- We pay _very_ careful attention in `processPendingInput()` to ensure
that we use either `this._runtimeItemPendingInput.code` or
`pendingInputLines`, both of which are guaranteed to always reflect the
current state of the world after the `await isCodeFragmentComplete()`
calls. Previously, the local variable version of `pendingInputLines` had
a strong chance to be outdated after the `await`s.

---

This might be a case where it would be useful to write a test that tries
to rapidly send some code to the console. If we can come up with a
failing test on main (even an intermittent failure would be fine), then
we can use that as a test case for this because if that _ever_ fails
after this PR then something is wrong.

This is the absolute simplest case I can come up with where mashing `Cmd
+ Enter` on each line _really really fast_ will sometimes trigger the
issue on `main`, so that is what I'd try and create a test against.

```r
Sys.sleep(.5)
print("hi there")
Sys.sleep(.5)

x <- 1
y <- x
z <- y
a <- z
```
  • Loading branch information
@DavisVaughan
DavisVaughan authored Aug 8, 2024
1 parent ef4f51c commit d8043ca
Showing 1 changed file with 122 additions and 58 deletions.
180 changes: 122 additions & 58 deletions src/vs/workbench/services/positronConsole/browser/positronConsoleService.ts
View file
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 @@ -1900,6 +1905,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 @@ -1924,90 +1934,144 @@ 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 {
// Need to `await` inside the `try` so that the `finally` only runs once
// `processPendingInputImpl()` has completely finished. Can't just return the promise.
await this.processPendingInputImpl();
} finally {
this._pendingInputState = 'Idle';
}
}

// 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') {
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

0 comments on commit d8043ca

Please sign in to comment.