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.

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

Attempt to safetly prevent duplicated execution commandline #24416

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
46 changes: 38 additions & 8 deletions src/client/common/terminal/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,26 @@
// Licensed under the MIT License.

import { inject, injectable } from 'inversify';
import { CancellationToken, Disposable, Event, EventEmitter, Terminal, TerminalShellExecution } from 'vscode';
import { CancellationToken, Disposable, Event, EventEmitter, Terminal, window } from 'vscode';
import '../../common/extensions';
import { IInterpreterService } from '../../interpreter/contracts';
import { IServiceContainer } from '../../ioc/types';
import { captureTelemetry } from '../../telemetry';
import { EventName } from '../../telemetry/constants';
import { ITerminalAutoActivation } from '../../terminals/types';
import { ICodeExecutionService, ITerminalAutoActivation } from '../../terminals/types';
import { ITerminalManager } from '../application/types';
import { _SCRIPTS_DIR } from '../process/internal/scripts/constants';
import { IConfigurationService, IDisposableRegistry } from '../types';
import {
IExecuteCommandResult,
ITerminalActivator,
ITerminalHelper,
ITerminalService,
TerminalCreationOptions,
TerminalShellType,
} from './types';
import { traceVerbose } from '../../logging';
import { getConfiguration } from '../vscodeApis/workspaceApis';

@injectable()
export class TerminalService implements ITerminalService, Disposable {
Expand Down Expand Up @@ -57,24 +59,31 @@ export class TerminalService implements ITerminalService, Disposable {
});
}
}
public async sendCommand(command: string, args: string[], _?: CancellationToken): Promise<void> {
public async sendCommand(
command: string,
args: string[],
_?: CancellationToken,
): Promise<IExecuteCommandResult | undefined> {
await this.ensureTerminal();
const text = this.terminalHelper.buildCommandForTerminal(this.terminalShellType, command, args);
if (!this.options?.hideFromUser) {
this.terminal!.show(true);
}

await this.executeCommand(text);
return this.executeCommand(text, false);
}
/** @deprecated */
public async sendText(text: string): Promise<void> {
await this.ensureTerminal();
if (!this.options?.hideFromUser) {
this.terminal!.show(true);
}
this.terminal!.sendText(text);
this.terminal!.sendText(text, false);
}
public async executeCommand(commandLine: string): Promise<TerminalShellExecution | undefined> {
public async executeCommand(
commandLine: string,
isPythonShell: boolean,
): Promise<IExecuteCommandResult | undefined> {
const terminal = this.terminal!;
if (!this.options?.hideFromUser) {
terminal.show(true);
Expand All @@ -97,11 +106,32 @@ export class TerminalService implements ITerminalService, Disposable {
});
await promise;
}
// If shell integration for python is disabled, use sendText inside REPL regardless of upstream shell integration setting.
const config = getConfiguration('python');
const pythonrcSetting = config.get<boolean>('terminal.shellIntegration.enabled');
if (isPythonShell && !pythonrcSetting) {
terminal.sendText(commandLine);
return undefined;
} else if (terminal.shellIntegration) {
// python in a shell , exit code is undefined . startCommand event happen, we call end command event
// TODO: Await the python REPL execute promise here. So we know python repl launched for sure before executing other python code.
// So we would not be interrupted.

// await this.serviceContainer.get<ICodeExecutionService>(ICodeExecutionService).replActive; getting undefined

if (terminal.shellIntegration) {
const execution = terminal.shellIntegration.executeCommand(commandLine);
traceVerbose(`Shell Integration is enabled, executeCommand: ${commandLine}`);
return execution;
return {
execution,
exitCode: new Promise((resolve) => {
const disposable = window.onDidEndTerminalShellExecution((event) => {
if (event.execution === execution) {
disposable.dispose();
resolve(event.exitCode);
}
});
}),
};
} else {
terminal.sendText(commandLine);
traceVerbose(`Shell Integration is disabled, sendText: ${commandLine}`);
Expand Down
13 changes: 8 additions & 5 deletions src/client/common/terminal/syncTerminalService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
'use strict';

import { inject } from 'inversify';
import { CancellationToken, Disposable, Event, TerminalShellExecution } from 'vscode';
import { CancellationToken, Disposable, Event } from 'vscode';
import { IInterpreterService } from '../../interpreter/contracts';
import { traceVerbose } from '../../logging';
import { PythonEnvironment } from '../../pythonEnvironments/info';
Expand All @@ -14,7 +14,7 @@ import * as internalScripts from '../process/internal/scripts';
import { createDeferred, Deferred } from '../utils/async';
import { noop } from '../utils/misc';
import { TerminalService } from './service';
import { ITerminalService } from './types';
import { IExecuteCommandResult, ITerminalService } from './types';

enum State {
notStarted = 0,
Expand Down Expand Up @@ -124,7 +124,7 @@ export class SynchronousTerminalService implements ITerminalService, Disposable
args: string[],
cancel?: CancellationToken,
swallowExceptions: boolean = true,
): Promise<void> {
): Promise<IExecuteCommandResult | undefined> {
if (!cancel) {
return this.terminalService.sendCommand(command, args);
}
Expand All @@ -145,8 +145,11 @@ export class SynchronousTerminalService implements ITerminalService, Disposable
public sendText(text: string): Promise<void> {
return this.terminalService.sendText(text);
}
public executeCommand(commandLine: string): Promise<TerminalShellExecution | undefined> {
return this.terminalService.executeCommand(commandLine);
public async executeCommand(
commandLine: string,
isPythonShell: boolean,
): Promise<IExecuteCommandResult | undefined> {
return this.terminalService.executeCommand(commandLine, isPythonShell);
}
public show(preserveFocus?: boolean | undefined): Promise<void> {
return this.terminalService.show(preserveFocus);
Expand Down
9 changes: 7 additions & 2 deletions src/client/common/terminal/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,20 @@ export interface ITerminalService extends IDisposable {
args: string[],
cancel?: CancellationToken,
swallowExceptions?: boolean,
): Promise<void>;
): Promise<IExecuteCommandResult | undefined>;
/** @deprecated */
sendText(text: string): Promise<void>;
executeCommand(commandLine: string): Promise<TerminalShellExecution | undefined>;
executeCommand(commandLine: string, isPythonShell: boolean): Promise<IExecuteCommandResult | undefined>;
show(preserveFocus?: boolean): Promise<void>;
}

export const ITerminalServiceFactory = Symbol('ITerminalServiceFactory');

export interface IExecuteCommandResult {
execution: TerminalShellExecution;
exitCode: Promise<number | undefined>;
}

export type TerminalCreationOptions = {
/**
* Object with environment variables that will be added to the Terminal.
Expand Down
15 changes: 12 additions & 3 deletions src/client/terminals/codeExecution/terminalCodeExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { sendTelemetryEvent } from '../../telemetry';
export class TerminalCodeExecutionProvider implements ICodeExecutionService {
private hasRanOutsideCurrentDrive = false;
protected terminalTitle!: string;
private replActive?: Promise<boolean>;
replActive?: Promise<boolean>;

constructor(
@inject(ITerminalServiceFactory) protected readonly terminalServiceFactory: ITerminalServiceFactory,
Expand Down Expand Up @@ -59,7 +59,7 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
this.configurationService.updateSetting('REPL.enableREPLSmartSend', false, resource);
}
} else {
await this.getTerminalService(resource).executeCommand(code);
await this.getTerminalService(resource).executeCommand(code, true);
}
}

Expand All @@ -73,6 +73,7 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
this.replActive = new Promise<boolean>(async (resolve) => {
const replCommandArgs = await this.getExecutableInfo(resource);
let listener: IDisposable;
// TODO: There's a race condition here as well; the send text from file command may happen before this timeout resolves
Promise.race([
new Promise<boolean>((resolve) => setTimeout(() => resolve(true), 3000)),
new Promise<boolean>((resolve) => {
Expand All @@ -99,9 +100,17 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
}
resolve(true);
});
// python
// python shell, exit code undefined.
// ondidStartShellExecution happens, si fires ondidEndsi

await terminalService.sendCommand(replCommandArgs.command, replCommandArgs.args);
// TODO: Store this promise and make sure no further commands are executed without awaiting .exitCode
const replExecution = await terminalService.sendCommand(replCommandArgs.command, replCommandArgs.args); // need to make sure this is resolved before starting executing something in repl.

// TODO: Should we await replyExecution.exitCode here?
await replExecution?.exitCode;
});

this.disposables.push(
terminalService.onDidCloseTerminal(() => {
this.replActive = undefined;
Expand Down
1 change: 1 addition & 0 deletions src/client/terminals/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { IDisposable, Resource } from '../common/types';
export const ICodeExecutionService = Symbol('ICodeExecutionService');

export interface ICodeExecutionService {
replActive?: Promise<boolean>;
execute(code: string, resource?: Uri): Promise<void>;
executeFile(file: Uri, options?: { newTerminalPerFile: boolean }): Promise<void>;
initializeRepl(resource?: Uri): Promise<void>;
Expand Down
Loading