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

Fix shell integration API reliability #22446

Merged
merged 2 commits into from
Nov 8, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Fix shell integration API reliability
  • Loading branch information
Kartik Raj committed Nov 8, 2023
commit 4b1b92b2f2943e66757d8f2b88691d0567a9e666
7 changes: 5 additions & 2 deletions src/client/terminals/envCollectionActivation/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
this.disposables,
);
const { shell } = this.applicationEnvironment;
const isActive = this.shellIntegrationService.isWorking(shell);
const isActive = await this.shellIntegrationService.isWorking(shell);
const shellType = identifyShellFromShellPath(shell);
if (!isActive && shellType !== TerminalShellType.commandPrompt) {
traceWarn(`Shell integration is not active, environment activated maybe overriden by the shell.`);
Expand All @@ -139,7 +139,10 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
location: ProgressLocation.Window,
title: Interpreters.activatingTerminals,
});
await this._applyCollectionImpl(resource, shell);
await this._applyCollectionImpl(resource, shell).catch((ex) => {
traceError(`Failed to apply terminal env vars`, shell, ex);
return Promise.reject(ex); // Ensures progress indicator does not disappear in case of errors, so we can catch issues faster.
});
this.progressService.hideProgress();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { identifyShellFromShellPath } from '../../common/terminal/shellDetectors
import { TerminalShellType } from '../../common/terminal/types';
import { createDeferred, sleep } from '../../common/utils/async';
import { cache } from '../../common/utils/decorators';
import { traceVerbose } from '../../logging';
import { traceError, traceVerbose } from '../../logging';
import { IShellIntegrationService } from '../types';

/**
Expand All @@ -30,8 +30,15 @@ export class ShellIntegrationService implements IShellIntegrationService {
@inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService,
) {}

@cache(-1, true)
public async isWorking(shell: string): Promise<boolean> {
return this._isWorking(shell).catch((ex) => {
traceError(`Failed to determine if shell supports shell integration`, shell, ex);
return false;
});
}

@cache(-1, true)
public async _isWorking(shell: string): Promise<boolean> {
const isEnabled = this.workspaceService
.getConfiguration('terminal')
.get<boolean>('integrated.shellIntegration.enabled')!;
Expand All @@ -50,19 +57,25 @@ export class ShellIntegrationService implements IShellIntegrationService {
// Proposed API is not available, assume shell integration is working at this point.
return true;
}
const disposable = onDidExecuteTerminalCommand((e) => {
if (e.terminal.name === name) {
deferred.resolve();
}
});
const terminal = this.terminalManager.createTerminal({
name,
shellPath: shell,
hideFromUser: true,
});
terminal.sendText(`echo ${shell}`);
const success = await Promise.race([sleep(3000).then(() => false), deferred.promise.then(() => true)]);
disposable.dispose();
return success;
try {
const disposable = onDidExecuteTerminalCommand((e) => {
if (e.terminal.name === name) {
deferred.resolve();
}
});
const terminal = this.terminalManager.createTerminal({
name,
shellPath: shell,
hideFromUser: true,
});
terminal.sendText(`echo ${shell}`);
const success = await Promise.race([sleep(3000).then(() => false), deferred.promise.then(() => true)]);
disposable.dispose();
return success;
} catch (ex) {
traceVerbose(`Proposed API is not available, failed to subscribe to onDidExecuteTerminalCommand`, ex);
// Proposed API is not available, assume shell integration is working at this point.
return true;
}
}
}
Loading