Feature Request: Terminal should listen for the WM_SETTINGCHANGE for environment variable updates

[rkeithhill]

Summary of the new feature/enhancement

When I install an app that updates the path or I update the path manually, it isn't enough to kill an individual tab in the Terminal and start a new one. The new tab still inherits the old, unchanged environment variable values. This means I have to kill/restart the Terminal and lose all my tabs.

Proposed technical implementation details (optional)

Add a Windows message handler for WM_SETTINGCHANGE and update the Terminal process's environment block so that any new tabs get the updated environment variables.

[ysc3839]

Related discussion in ConEmu. Maximus5/ConEmu#468

[EdinaLewis]

Opening a new tab should definitely inherit from the current system environment.

[zadjii-msft]

<This was a kind of unfocused train of thought, though I'm happy with the conclusion at the bottom. leaving my own notes so I can remember whenever I get back to this>

Huh, this might be a bit trickier than I thought. WM_SETTINGCHANGE doesn't tell you which variable changed, only that the "Environment" changed. If we call GetEnvironmentStrings, we're still going to just get our launch environment variables.

We could call CreateEnvironmentBlock with nullptr as the hToken to get a fresh system environment block, but that's probably wrong, since we want the user's environment block. That involves also calling LogonUser. That's presumably doable.

Though, if the user launched the Terminal from one process with some extra variables set, and expected it to inherit variables from the parent process, then we probably shouldn't blow away the env vars for the first tab that's created. I suppose we could only do this refreshing of the environment block when we do get a WM_SETTINGCHANGE. Subsequent tabs would all use the user's default env block, not the one inherited from the parent. That's a little funky - it sorta creates scenarios that aren't totally expected, because sometimes the parent's values would persist to subsequent tabs.

I guess the most unified solution would be to just use a fresh environment block for all connections created after startup. That would at least be consistent behavior. We'd probably want to wait for #4023 to merge first, and set some internal flag to use a fresh env block, only after all the startup actions are processed. Then we wouldn't even need to use WM_SETTINGCHANGE, we'd just assume that you wanted a fresh one.

[yitzhaks]

Opening a new tab should definitely inherit from the current system environment.

nit: I would suggest the current user environment. explorer.exe does this correctly when launching new processes, so I hope it's not too complicated.

[DHowett-MSFT]

Make sure when we reload this, we also reload settings. That'll probably fix the keyboard issue in 4735.

[garretwilson]

$env:Path = [System.Environment]::GetEnvironmentVariable("Path","Machine") + ";" + [System.Environment]::GetEnvironmentVariable("Path","User")

I had almost filed a bug with Node.js, then almost filed a bug with PowerShell, until I finally found this ticket. I had found the same workaround that @dexeonify mentioned.

Yes, I can see putting that workaround in my PowerShell $PROFILE so that every time I start PowerShell I know I'm getting the "one true path" (although I don't like the way that sounds, now that I think about it); but … really? Am I misreading things, or has this ticket been unresolved for over three years?!

Is this on any team's schedule to address? I think most people here would agree that just reloading Path would be a major step forward. The current situation is only furthering Windows' reputation that "I have to reboot after the slightest change for things to work properly".

[garretwilson]

I've read most of the comments here, and I still don't get why closing the entire Microsoft Terminal instance and restarting it doesn't result in the latest Path environment variable being loaded. Surely Microsoft Terminal doesn't keep running in the background, does it?

[bruno-brant]

It does

[zadjii-msft]

It does

It sure shouldn't be. There might be a lasting RuntimeBroker.exe, but I don't think that should have anything to do with it. Closing all the Terminal windows (s.t. there are no windowsterminal.exe's running) should cause the next Terminal launch to have a fresh env block.

[garretwilson]

Closing all the Terminal windows (s.t. there are no windowsterminal.exe's running) should cause the next Terminal launch to have a fresh env block.

But alas … it doth not. 😒

I see various "Console Windows Host" entries in Task Manager. Would that have anything to do with it? If so, does that mean there's yet another bug that Microsoft Terminal isn't shutting down properly?

[char-46]

Now this problem more seriously.
Because of this program add a feature that can merge new window to existing window, so that I can't use Run command to create a new task to reload environment variable.

[cforce]

I can start a new cmd from windows start menu and my user PATH is loaded
If i do the same ( same cmd and params) via terminal it doesn't reflect the PATH changes.
I can open a dozens new terminal windows cmd windows from the same running terminal parent instance - issue still remains.
It seems to recover when i completly terminate the terminal parent and create a new and a new cmd window . so its terminal app caching the context. Do you use some cached state of context?
How to enforce REAL (re)loadind onf env?
My workaround is now to use choco's "refreshenv" .. but5 hey this is definitely a bug in the terminal application so please reopen this issue.

[zadjii-msft]

so please reopen this issue.

Thanks for the feedback! As you'll notice, this issue is currently open. We've got an idea how to solve this with the work being done in #12516, now we just need to book some time to polish that out.

If anyone is interested in helping out, I'd be more than happy to help give an outline of what I was planning on doing with that thread. It'll unfortunately be a few months before I can loop back on that, so if folks want this sooner than that, I'd appreciate the help ☺️

[zadjii-msft]

Note

Walkthrough

Okay, if I was gonna do this, here's how I'd approach this. I'd make it two PRs. One to finish building the helper class, and one to actually integrate it into the Teminal

• Finish the til::env PR, Create til::env to help with refreshing environment variables #12516.
  • This was intended to be a helper that would generate an env block similar to the way that the Windows shell does when launching new processes from explorer.exe.
  • I honestly don't know what's missing from the PR. Fortunately, @miniksa is still a close friend of the team, so I think he'd be more than willing to give some pointers.

  • it was pretty much done except for me updating the spell check and perhaps adding more tests

  • "please stop inheriting from std::map"

    • In the current incarnation of that PR, it directly derives from std::map, which is generally frowned upon. Find a way to implement it without that. Probably fine to just move the map into a private member variable.
• Generate fresh environment blocks for every connection (tab, pane, etc)
  • Use a til::env to generate the new env block.
  • TerminalPage::_CreateConnectionFromSettings (in TerminalPage.cpp) is the method responsible for creating new ConptyConnections (which is what's used to hook the Terminal up to a cmd.exe or other commandline process)
    • In that method, we're already injecting a couple env vars

      StringMap envMap{};
      envMap.Insert(L"WT_PROFILE_ID", guidWString);
      envMap.Insert(L"WSLENV", L"WT_PROFILE_ID");

      So that seems like about the right place to make changes to me.
  • We should also add a global setting to allow folks to opt-out of this behavior (in case we break someone's workflow)
    • This should be stored in the Global settings, so you could add a bool to the MTSM_GLOBAL_SETTINGS x-macro in MTSMSettings.h.
    • Name it something like experimental.reloadEnvironmentVariables, and default it to true.

I honestly think that's it. That should just work. We shouldn't need to listen for env var updates, because we should just get a fresh block everytime.

And then we can handle ourselves the follow-up of merging #9287 with the code written to solve this issue.

[miniksa]

AFAIK, it was pretty much done except for me updating the spell check and perhaps adding more tests. It probably just needs a spit shine then check it in and call anything else a bug, @zadjii-msft.

[DHowett]

Ah, plus me imploring you to "please stop inheriting from std::map". Thanks for the update 😄