vscode.env.shell might return empty string to extension if fetched very soon after startup

[gjsjohnmurray]

Type: Bug

Seems to be a timing issue. The following steps sometimes reproduce it for me, but success may depend on the performance of your workstation (mine is Windows) and which other extensions are enabled by default.

1. Clone https://github.com/gjsjohnmurray/vscode-extension-samples/tree/show-160694
2. Open the statusbar-sample folder in VS Code
3. Open a Terminal in that folder and run npm install
4. Switch to Debug view, choose the ONLY Extension configuration and run it. This launches with --disable-extensions
5. When Extension Host window appears, look on the right of the status bar. If the repro was successful it will look like this:

If this doesn't happen, try the next steps:

6. End debugging and launch the Extension configuration instead. This launches normally and probably won't trigger the problem:

7. From Command Palette of the EH window, run Developer: Reload With Extensions Disabled. With luck this will cause the problem to happen (see first screenshot).

The extension activates "*", so immediately on startup. The first attempt to get vscode.env.shell happens from its activate method.

If the problem shows, click the status bar panel to get the value again. This time it should succeed.

VS Code version: Code - Insiders 1.72.0-insider (835ace5, 2022-09-12T05:16:16.290Z)
OS version: Windows_NT x64 10.0.22000
Modes:
Sandboxed: Yes

[jrieken]

@Tyriar Looks like you should be using extension host init data for this, not a dynamic value that changes along the way

[Tyriar]

@jrieken wouldn't that mean blocking the extension host launch on the pty host and its profile detection round trip? I believe this has always been like this and the value is dynamic depending on settings/environment.

[jrieken]

yes - either block or make it clear that the value changes, e.g we need an event to signal changes. I am not familiar with how this works, does the shell-value change during a single session?

[Tyriar]

Changing the default shell (via settings or UI) will change the value. Let's track adding this API in this issue:

export namespace env {
	export const onDidChangeShell: Event<string>;
}

[gjsjohnmurray]

Would an early-activating extension expect to receive this event when the value of env.shell changes from its apparent initial value of emptystring to whatever shell is the initial default?

As I understand it, for some modes of VS Code operation env.shell will always be emptystring. So my extension would probably have to do other tests to check for those, otherwise it could wait forever for an event telling it that the initial shell is now known.

[Tyriar]

@gjsjohnmurray yes env.shell may be undefined, for example in vscode.dev. Not sure what you're doing exactly but you could listen on env.onDidChangeShell and enable the functionality at that point, or after some timeout? Our AutoOpenBarrier class in core would be handy to implement that

vscode/src/vs/base/common/async.ts

Lines 438 to 455 in 9b80ed6

	/**
	* A barrier that is initially closed and then becomes opened permanently after a certain period of
	* time or when open is called explicitly
	*/
	export class AutoOpenBarrier extends Barrier {
	private readonly _timeout: any;
	constructor(autoOpenTimeMs: number) {
	super();
	this._timeout = setTimeout(() => this.open(), autoOpenTimeMs);
	}
	override open(): void {
	clearTimeout(this._timeout);
	super.open();
	}
	}

In the meantime I recommend you poll for the initial value and give up after some time, you may also want to re-check the value at a later point as well as it can change.

[babakks]

@Tyriar Submitted a PR for this. Do you think that works?

[jrieken]

Leaving this open for future finalization

[gjsjohnmurray]

Not sure what you're doing exactly

@Tyriar from the activate method of the extension we call window.createTerminal and use its sendText method to run commands. When composing the command string we check window.env.shell to discover what shell we'll be interacting with.

We're working around the problem by retrying the window.env.shell call a few times. The proposed API should allow a different strategy in future, but we might just stay with what we're doing.

[gjsjohnmurray]

I think my job would have been easier if the Terminal returned by window.createTerminal had a shell property that told me what shell it was using. Currently it has a creationOptions property from which I can only discover the shell if window.createTerminal had specified shellPath explicitly.

[Tyriar]

Update since this was last discussed: the pty host now doesn't necessarily start up, however profiles are now always detected on the main/server procs, not the pty host:

vscode/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts

Lines 294 to 296 in 29d8134

	async getProfiles(profiles: unknown, defaultProfile: unknown, includeDetectedProfiles?: boolean) {
	return this._localPtyService.getProfiles(this._workspaceContextService.getWorkspace().id, profiles, defaultProfile, includeDetectedProfiles) || [];
	}

Needs a final decision whether we want to block the exthost startup on terminal profile resolving.

[Tyriar]

I measured the time it takes to getProfiles on the main process which is necessary in order to get the default shell and it came out to 55ms. Given this, I still think the impact is too great when doing this eagerly on the main process during startup and before the extension host is launched.

2023-08-22 04:30:18.840 [info] getProfiles start
2023-08-22 04:30:18.890 [trace] [File Watcher (node.js)] [raw] ["change"] globalStorage
2023-08-22 04:30:18.891 [trace] [File Watcher (node.js)] [CHANGED] c:\Users\Daniel\AppData\Roaming\code-oss-dev\User\globalStorage
2023-08-22 04:30:18.891 [trace] [File Watcher (node.js)] [raw] ["change"] globalStorage
2023-08-22 04:30:18.891 [trace] [File Watcher (node.js)] [CHANGED] c:\Users\Daniel\AppData\Roaming\code-oss-dev\User\globalStorage
2023-08-22 04:30:18.893 [trace] [File Watcher (node.js)] [raw] ["change"] globalStorage
2023-08-22 04:30:18.893 [trace] [File Watcher (node.js)] [CHANGED] c:\Users\Daniel\AppData\Roaming\code-oss-dev\User\globalStorage
2023-08-22 04:30:18.894 [trace] [File Watcher (node.js)] [raw] ["change"] globalStorage
2023-08-22 04:30:18.894 [trace] [File Watcher (node.js)] [CHANGED] c:\Users\Daniel\AppData\Roaming\code-oss-dev\User\globalStorage
2023-08-22 04:30:18.895 [info] getProfiles end

Caching it optimistically across sessions is a good idea though. Created #190974 to track that separately from the event.

[jrieken]

Can we put these numbers in perspective, e.g how about setting perf-markers so that they make it into telemetry so that we have a better view. 50ms isn't slow because from main, we start the renderer, we load the renderer code which are all opportunities to run this in parallel so that the results from getProfiles are ready before we even attempt to start the EH process (which usually happens ~2sec after starting main)

[jrieken]

Let's leave this issue open because shell is still empty after startup but now we have the event which also serves as a workaround

[Tyriar]

I had a look at this and it's not as easy as I expected to get the profile in main as currently it depends upon the workspace and the renderer process evaluating parts of it before being handed off the the main process. I'm closing this as finalized because the issue where the initial value is an empty string is separate from the event API, plus it's tracked in the targeted issue #190974