Do not wait for shell environment resolution before opening window

[joaomoreno]

Details: #106537 (comment)

Today: The workbench depends on user env to be resolved. Resolving user env envolves spawning a shell and executing the user's init scripts. This can take long.

Ideally: The user env is only required for specific features: opening terminals, spawning tasks, spawning extension host. Until it's resolved, many other features should actually work, ie explorer, editor, etc.

Related:

• Support per-window variable environments #15452
• Preserve per-window environment variables between workspace changes #108571
• Correctly inherit/don't inherit the shell environment #55194
• Path issues with -i #30847

Adoption

I made a pass over usages of process environment by looking at:

• direct use of process.env
• usages of IProcessEnvironment
• clients of vs/base/parts/ipc/node/ipc.cp that spawn child processes

Assigning people based on areas and usages. Feel free to remove yourself and your items when this change needs no adoption in your area.

• Terminal @Tyriar @meganrogge

  • vs/base/node/shell.ts @TylerLeonhardt

• Debug @weinand

  • vs/workbench/contrib/debug/browser/rawDebugSession.ts
  • vs/workbench/contrib/debug/node/debugAdapter.ts (only runs in EH -> nothing to do)

• Tasks @alexr00

  • vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts
  • vs/workbench/services/configurationResolver/electron-sandbox/configurationResolverService.ts

• Proxy @chrmarti

• vs/platform/request/node/proxy.ts

• vs/workbench/services/extensions/node/proxyResolver.ts

Child Processes

• search @roblourens
• telemetry @sbatten

[bpasero]

After discussion with @joaomoreno we think this could work:

• we stop replacing process.env in the main and renderer processes, but instead keep it as is
• we introduce a new service IProcessEnvService that has a single method getProcessEnv(): Promise<typeof process.env>

Depending the platform, the implementation will talk to the electron-main side and ask for a merged flat view on the process environment to use:

• if the window was started from a shell (CLI) it will be the snapshot of the shell environment
  • a window will preserve this environment even when it changes its opened folder to fix Preserve per-window environment variables between workspace changes #108571
• if the window was started from the UI it will be the base renderer process.env overwritten with a shell environment which can come from one of two sources:
  • if main was started from a shell, it's that initial environment
  • if main was started from the UI, we probe the user's shell for an environment by spawning process.env.SHELL (this is the
    only reason why getProcessEnv() needs to return a promise)

Adoption needs to take place anywhere we want this merged flat view of the environment, e.g. when we spawn the extension host but also in other places. The challenge is that the environment will be a Promise and as such, previous sync code that used process.env will have to be async going forward.

[segevfiner]

If the first launch of VS Code started from CLI, under let's say some Python virtualenv, then any new window created not from CLI will have that virtualenv applied. That's unlike the case when the first launch of VS Code was from UI, where it would just have the shell environment.

What I'm trying to say is that anything that depends on whatever the first launch happened to be is confusing. It might make more sense for it to only depend on the way the current launch happened, independent of whatever the first launch happened to be. But that's just my two cents, there may be a reason for it that I'm not seeing.

Or maybe that first launch thing is really for when you launch a new VS Code window from another VS Code window, rather then the OS GUI. Maybe it would make more sense to inherit the environment of that window then, instead of the environment of the first launch.

Any work on improving the environment variables situation is more than welcome anyhow.

[bpasero]

@segevfiner that could be seen as breaking behaviour though, no? I wonder if we could have a setting that enforces that every window that opens after the first one uses the shell environment and does not inherit the first window environment if that was launched from the terminal.

Nevertheless, that still might need careful explanation as to what happens when you change the folder from within the window. Some users might expect the setting to apply there too and some maybe not.

[joaomoreno]

(Unrelated: @segevfiner can you send me a hello email to joao moreno at my company com?)

[segevfiner]

@segevfiner that could be seen as breaking behaviour though, no? I wonder if we could have a setting that enforces that every window that opens after the first one uses the shell environment and does not inherit the first window environment if that was launched from the terminal.

Nevertheless, that still might need careful explanation as to what happens when you change the folder from within the window. Some users might expect the setting to apply there too and some maybe not.

Could be, not sure who may or may not depend on this. I'm just basing this on how I expect it to behave, with each window just taking the environment from whatever way you launch. The first launch thing is really an implementation detail IMHO.

And yeah there is also the case of changing a folder in an existing window as opposed to a new one. You described just preserving the environment the window started with (Unrelated to the first launch), which makes sense.

[bpasero]

I just think it could be frustrating to setup a nice environment when you start VSCode from the terminal and then only getting it in the first window. Don't forget that we do potentially restore more than one window on startup, so in that case we cannot just apply the environment only to the first window and treat the others as new window. This would need a lot of careful tweaking to get right and I am not sure it is worth it and may be more confusing than helpful. At least today we have a somewhat consistent behaviour.

[segevfiner]

(Unrelated: @joaomoreno Not sure I follow what that email address is, but you can find my gmail email address on my commits)

[segevfiner]

@bpasero There are cases for both behaviours I guess. On restoring a session you indeed want the environment appplied to all the restored windows.

But also imagine keeping VS Code running for a long time then opening a new window and getting some virtualenv from something that you worked on long ago that happened to be the first launch. I think in macOS it might even keep that process/VS Code open even when no windows are open (Not sure it does so currently).

[alexr00]

I think this will not be adoptable in the configuration resolver service (used for ${variable} style variables in various places we have json, including some settings). Variable resolving must remain sync since some settings use it, so I don't see how we could wait for the promise to resolve.

[bpasero]

@alexr00 can you clarify what kind of process.env the configuration resolver service is using?

[joaomoreno]

@alexr00 Couldn't these settings assume the pre-resolved values at first and then emit configuration change events whenever the user shell env gets resolved?

[alexr00]

@bpasero one of the possible variables is ${env:<name of whatever environment variable you want>}.

@joaomoreno it looks like might only be terminal setting that use this. I would need to defer to the settings owners to know if that would be ok.

[bpasero]

Since I am not sure we can adopt this in a timely manner, I have meanwhile pushed a couple of changes to master to address the biggest pain points. After these changes, the introduction of IProcessEnvService will only be a performance fix (in some cases), but not fix any other issues.

• the timeout for resolving the shell environment is back to 10s (from 3s) but a warning will be shown to the user when this timeout is hit pointing to some docs (we still have to write) to guide the user into fixing this (via 6fdc79d)
• a new perf metric ellapsedWaitForShellEnv is added so that users who see slow startups from resolving shell env can be better identified (via fe65b26)
• the code to resolve and merge shell env has moved into bootstrap-window.js and thus will now run in all of our windows (workbench, shared process, issue reporter, process explorer) and the shell config will no longer overwrite the user config (6f3fcd2)
• a second window that was started from a CLI will no longer trigger the shell environment resolution if the first window was started from the UI by taking the second window's environment into account (fb277ed)
• the user env of a window will now be preserved even when the window loads a different folder (ff1887b)

[weinand]

@bpasero the only reason why process.env was accessed in vs/workbench/contrib/debug/browser/rawDebugSession.ts was the "launch.json" feature to delete environment variables by using the value null. The code in rawDebugSession.ts merged and deleted the environment variables provided by the launch.json.

When reviewing the implementation, I noticed that the delete part no longer worked: it seems that another merge happening later undoes the delete.

I've removed the no-longer working process.env code in rawDebugSession.ts and we will have to find a better way to implement the deletion of environment variables. See #120560

[bpasero]

I have looked at the telemetryApp and think it should be fine because of the work that happened in ICustomEndpointTelemetryService where the app runs as a child from the shared process that gets the shell environment.

The remaining work is in terminals and configuration resolvers 👍

[Tyriar]

Pretty sure terminal done now 🎉

[alexr00]

With some help from @Tyriar, I'm pretty sure that the configuration resolver is also done now.

[bpasero]

Thanks a lot ❤️

[bpasero]

We will take out the process environment patching early May to get a good milestone of coverage.

[ghost]

Just out of curiosity, is there any way to just permanently ignore the "taking too long" warning message..?

I ask because I'm required to use a Mac at work (I really prefer Linux myself), but the machine I've been given to use is a late 2013 Intel i5 iMac 27". This thing is stupidly slow at the best of times, I've timed a cold boot at a little over 3 minutes, multiple times.

So I know it's slow, I don't need another notification telling me that every time I open a VSCode instance, which quite often involves multiple instances at the same time..

[bpasero]

Just out of curiosity, is there any way to just permanently ignore the "taking too long" warning message..?

Yes, switch to insiders where this notification no longer appears: https://code.visualstudio.com/insiders/