Skip to content

Commit

Permalink
Remove shell environment patching in the renderer (fix #108804)
Browse files Browse the repository at this point in the history
  • Loading branch information
@bpasero
bpasero committed May 3, 2021
1 parent 16baed2 commit 89731c8
Show file tree
Hide file tree
Showing 7 changed files with 36 additions and 115 deletions.
7 changes: 0 additions & 7 deletions src/bootstrap-window.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,13 +179,6 @@
require(modulePaths, async result => {
try {

// Wait for process environment being fully resolved
performance.mark('code/willWaitForShellEnv');
if (!safeProcess.env['VSCODE_SKIP_PROCESS_ENV_PATCHING'] /* TODO@bpasero for https://github.com/microsoft/vscode/issues/108804 */) {
await safeProcess.shellEnv();
}
performance.mark('code/didWaitForShellEnv');

// Callback only after process environment is resolved
const callbackResult = resultCallback(result, configuration);
if (callbackResult instanceof Promise) {
Expand Down
6 changes: 0 additions & 6 deletions src/vs/base/parts/sandbox/electron-browser/preload.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,6 @@
ipcRenderer.invoke('vscode:fetchShellEnv')
]);

if (!process.env['VSCODE_SKIP_PROCESS_ENV_PATCHING'] /* TODO@bpasero for https://github.com/microsoft/vscode/issues/108804 */) {
// Assign all keys of the shell environment to our process environment
// But make sure that the user environment wins in the end over shell environment
Object.assign(process.env, shellEnv, userEnv);
}

return { ...process.env, ...shellEnv, ...userEnv };
})();

Expand Down
95 changes: 31 additions & 64 deletions src/vs/code/electron-main/app.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ import { IKeyboardLayoutMainService, KeyboardLayoutMainService } from 'vs/platfo
import { NativeParsedArgs } from 'vs/platform/environment/common/argv';
import { isLaunchedFromCli } from 'vs/platform/environment/node/argvHelper';
import { isEqualOrParent } from 'vs/base/common/extpath';
import { CancellationToken, CancellationTokenSource } from 'vs/base/common/cancellation';
import { RunOnceScheduler } from 'vs/base/common/async';
import { IExtensionUrlTrustService } from 'vs/platform/extensionManagement/common/extensionUrlTrust';
import { ExtensionUrlTrustService } from 'vs/platform/extensionManagement/node/extensionUrlTrustService';
import { once } from 'vs/base/common/functional';
Expand Down Expand Up @@ -282,71 +282,29 @@ export class CodeApplication extends Disposable {

//#region Bootstrap IPC Handlers

let slowShellResolveWarningShown = false;
ipcMain.handle('vscode:fetchShellEnv', event => {
return new Promise(async resolve => {

// DO NOT remove: not only usual windows are fetching the
// shell environment but also shared process, issue reporter
// etc, so we need to reply via `webContents` always
const webContents = event.sender;

let replied = false;

function acceptShellEnv(env: IProcessEnvironment): void {
clearTimeout(shellEnvSlowWarningHandle);
clearTimeout(shellEnvTimeoutErrorHandle);

if (!replied) {
replied = true;

if (!webContents.isDestroyed()) {
resolve(env);
}
}
}

// Handle slow shell environment resolve calls:
// - a warning after 3s but continue to resolve (only once in active window)
// - an error after 10s and stop trying to resolve (in every window where this happens)
const cts = new CancellationTokenSource();

const shellEnvSlowWarningHandle = setTimeout(() => {
if (!slowShellResolveWarningShown) {
this.windowsMainService?.sendToFocused('vscode:showShellEnvSlowWarning', cts.token);
slowShellResolveWarningShown = true;
}
}, 3000);

const window = this.windowsMainService?.getWindowByWebContents(event.sender); // Note: this can be `undefined` for the shared process!!
const shellEnvTimeoutErrorHandle = setTimeout(() => {
cts.dispose(true);
window?.sendWhenReady('vscode:showShellEnvTimeoutError', CancellationToken.None);
acceptShellEnv({});
}, 10000);

// Prefer to use the args and env from the target window
// when resolving the shell env. It is possible that
// a first window was opened from the UI but a second
// from the CLI and that has implications for whether to
// resolve the shell environment or not.
//
// Window can be undefined for e.g. the shared process
// that is not part of our windows registry!
let args: NativeParsedArgs;
let env: IProcessEnvironment;
if (window?.config) {
args = window.config;
env = { ...process.env, ...window.config.userEnv };
} else {
args = this.environmentMainService.args;
env = process.env;
}
// Prefer to use the args and env from the target window
// when resolving the shell env. It is possible that
// a first window was opened from the UI but a second
// from the CLI and that has implications for whether to
// resolve the shell environment or not.
//
// Window can be undefined for e.g. the shared process
// that is not part of our windows registry!
const window = this.windowsMainService?.getWindowByWebContents(event.sender); // Note: this can be `undefined` for the shared process
let args: NativeParsedArgs;
let env: IProcessEnvironment;
if (window?.config) {
args = window.config;
env = { ...process.env, ...window.config.userEnv };
} else {
args = this.environmentMainService.args;
env = process.env;
}

// Resolve shell env
const shellEnv = await resolveShellEnv(this.logService, args, env);
acceptShellEnv(shellEnv);
});
// Resolve shell env
return resolveShellEnv(this.logService, args, env);
});

ipcMain.handle('vscode:writeNlsFile', (event, path: unknown, data: unknown) => {
Expand Down Expand Up @@ -1012,7 +970,16 @@ export class CodeApplication extends Disposable {
}

// Start to fetch shell environment (if needed) after window has opened
resolveShellEnv(this.logService, this.environmentMainService.args, process.env);
// Since this operation can take a long time, we want to warm it up while
// the window is opening.
// We also print a warning if the resolution takes longer than 10s.
(async () => {
const slowResolveShellEnvWarning = this._register(new RunOnceScheduler(() => this.logService.warn('Resolving your shell environment is taking more than 10s. Please review your shell configuration. Learn more at https://go.microsoft.com/fwlink/?linkid=2149667.'), 10000));
slowResolveShellEnvWarning.schedule();

await resolveShellEnv(this.logService, this.environmentMainService.args, process.env);
slowResolveShellEnvWarning.dispose();
})();

// If enable-crash-reporter argv is undefined then this is a fresh start,
// based on telemetry.enableCrashreporter settings, generate a UUID which
Expand Down
7 changes: 4 additions & 3 deletions src/vs/workbench/contrib/files/browser/editors/binaryFileEditor.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,14 @@ export class BinaryFileEditor extends BaseBinaryResourceEditor {
// Try to let the user pick an override if there is one availabe
const overridenInput = await this.editorOverrideService.resolveEditorOverride(input, { ...options, override: EditorOverride.PICK, }, this.group);

let newOptions = overridenInput?.options ?? options;
newOptions = { ...newOptions, override: EditorOverride.DISABLED };
// Replace the overrriden input, with the text based input
await this.editorService.replaceEditors([{
editor: input,
replacement: overridenInput?.editor ?? input,
options: newOptions,
options: {
...overridenInput?.options ?? options,
override: EditorOverride.DISABLED
}
}], overridenInput?.group ?? this.group);
}
}
Expand Down
1 change: 0 additions & 1 deletion src/vs/workbench/contrib/performance/browser/perfviewEditor.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,6 @@ class PerfModelContentProvider implements ITextModelContentProvider {
table.push(['window.loadUrl() => begin to require(workbench.desktop.main.js)', metrics.timers.ellapsedWindowLoadToRequire, '[main->renderer]', StartupKindToString(metrics.windowKind)]);
table.push(['require(workbench.desktop.main.js)', metrics.timers.ellapsedRequire, '[renderer]', `cached data: ${(metrics.didUseCachedData ? 'YES' : 'NO')}${stats ? `, node_modules took ${stats.nodeRequireTotal}ms` : ''}`]);
table.push(['wait for window config', metrics.timers.ellapsedWaitForWindowConfig, '[renderer]', undefined]);
table.push(['wait for shell environment', metrics.timers.ellapsedWaitForShellEnv, '[renderer]', undefined]);
table.push(['init storage (global & workspace)', metrics.timers.ellapsedStorageInit, '[renderer]', undefined]);
table.push(['init workspace service', metrics.timers.ellapsedWorkspaceServiceInit, '[renderer]', undefined]);
if (isWeb) {
Expand Down
24 changes: 1 addition & 23 deletions src/vs/workbench/electron-sandbox/window.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import { IWorkspaceFolderCreationData } from 'vs/platform/workspaces/common/work
import { IIntegrityService } from 'vs/workbench/services/integrity/common/integrity';
import { isWindows, isMacintosh } from 'vs/base/common/platform';
import { IProductService } from 'vs/platform/product/common/productService';
import { INotificationService, IPromptChoice, NeverShowAgainScope, Severity } from 'vs/platform/notification/common/notification';
import { INotificationService, Severity } from 'vs/platform/notification/common/notification';
import { IKeybindingService } from 'vs/platform/keybinding/common/keybinding';
import { INativeWorkbenchEnvironmentService } from 'vs/workbench/services/environment/electron-sandbox/environmentService';
import { IAccessibilityService, AccessibilitySupport } from 'vs/platform/accessibility/common/accessibility';
Expand Down Expand Up @@ -186,28 +186,6 @@ export class NativeWindow extends Disposable {
// Message support
ipcRenderer.on('vscode:showInfoMessage', (event: unknown, message: string) => this.notificationService.info(message));

// Shell Environment Issue Notifications
const choices: IPromptChoice[] = [{
label: localize('learnMore', "Learn More"),
run: () => this.openerService.open('https://go.microsoft.com/fwlink/?linkid=2149667')
}];

ipcRenderer.on('vscode:showShellEnvSlowWarning', () => this.notificationService.prompt(
Severity.Warning,
localize('shellEnvSlowWarning', "Resolving your shell environment is taking very long. Please review your shell configuration."),
choices,
{
sticky: true,
neverShowAgain: { id: 'ignoreShellEnvSlowWarning', scope: NeverShowAgainScope.GLOBAL }
}
));

ipcRenderer.on('vscode:showShellEnvTimeoutError', () => this.notificationService.prompt(
Severity.Error,
localize('shellEnvTimeoutError', "Unable to resolve your shell environment in a reasonable time. Please review your shell configuration."),
choices
));

// Fullscreen Events
ipcRenderer.on('vscode:enterFullScreen', async () => {
await this.lifecycleService.when(LifecyclePhase.Ready);
Expand Down
11 changes: 0 additions & 11 deletions src/vs/workbench/services/timer/browser/timerService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ export interface IMemoryInfo {
"timers.ellapsedWindowLoad" : { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "isMeasurement": true },
"timers.ellapsedWindowLoadToRequire" : { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "isMeasurement": true },
"timers.ellapsedWaitForWindowConfig" : { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "isMeasurement": true },
"timers.ellapsedWaitForShellEnv" : { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "isMeasurement": true },
"timers.ellapsedStorageInit" : { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "isMeasurement": true },
"timers.ellapsedWorkspaceServiceInit" : { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "isMeasurement": true },
"timers.ellapsedSharedProcesConnected" : { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "isMeasurement": true },
Expand Down Expand Up @@ -261,15 +260,6 @@ export interface IStartupMetrics {
*/
readonly ellapsedWaitForWindowConfig: number;

/**
* The time it took to wait for resolving the shell environment. This time the workbench
* will not continue to load and be blocked entirely.
*
* * Happens in the renderer-process
* * Measured with the `willWaitForShellEnv` and `didWaitForShellEnv` performance marks.
*/
readonly ellapsedWaitForShellEnv: number;

/**
* The time it took to init the storage database connection from the workbench.
*
Expand Down Expand Up @@ -593,7 +583,6 @@ export abstract class AbstractTimerService implements ITimerService {
ellapsedWindowLoadToRequire: this._marks.getDuration('code/willOpenNewWindow', 'code/willLoadWorkbenchMain'),
ellapsedRequire: this._marks.getDuration('code/willLoadWorkbenchMain', 'code/didLoadWorkbenchMain'),
ellapsedWaitForWindowConfig: this._marks.getDuration('code/willWaitForWindowConfig', 'code/didWaitForWindowConfig'),
ellapsedWaitForShellEnv: this._marks.getDuration('code/willWaitForShellEnv', 'code/didWaitForShellEnv'),
ellapsedStorageInit: this._marks.getDuration('code/willInitStorage', 'code/didInitStorage'),
ellapsedSharedProcesConnected: this._marks.getDuration('code/willConnectSharedProcess', 'code/didConnectSharedProcess'),
ellapsedWorkspaceServiceInit: this._marks.getDuration('code/willInitWorkspaceService', 'code/didInitWorkspaceService'),
Expand Down

0 comments on commit 89731c8

Please sign in to comment.