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

Setting startingDirectory to WSL path causes indefinite application hang #9541

Closed
zaxbux opened this issue Mar 18, 2021 · 12 comments · Fixed by #10045
Closed

Setting startingDirectory to WSL path causes indefinite application hang #9541

zaxbux opened this issue Mar 18, 2021 · 12 comments · Fixed by #10045
Labels
Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@zaxbux
Copy link

zaxbux commented Mar 18, 2021

Environment

Windows build number: 19041.867
Windows Terminal version (if applicable): 1.6.10571.0

Steps to reproduce

Add the following startingDirectory to a profile like so:

{
  "guid": "{07b52e3e-de2c-5db4-bd2d-ba144ed6c273}",
  "hidden": false,
  "name": "Ubuntu-20.04",
  "source": "Windows.Terminal.Wsl",
  "startingDirectory": "\\\\wsl$\\Ubuntu-20.04\\home\\zach"
}

Expected behavior

Terminal launches profile in starting directory. This has worked for awhile, issue started occuring after some recent updates (either Windows or Terminal).

Actual behavior

Terminal hangs forever when starting profile from new tab dropdown, jumplist, etc.

wt_bug

Note: this screenshot was taken after the startingDirectory was added in my settings.json while application was running. Terminal crashes as soon as the updated profile was clicked. Terminal launched fine and profile worked before adding line.

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Mar 18, 2021
@DHowett
Copy link
Member

DHowett commented Mar 18, 2021

Sorry -- all we do is pass that path off to CreateProcess. If the WSL interop implementation is hanging, it'll be bad...

I wonder if Terminal can/should have a process startup watchdog? "[process failed to start in 10 seconds; you likely don't want to try to continue]"

@Nacimota
Copy link
Contributor

I wonder if Terminal can/should have a process startup watchdog? "[process failed to start in 10 seconds; you likely don't want to try to continue]"

Separate but related: WSL2 takes about 5 seconds to start up cold on my system, and I notice the terminal UI hangs for the duration.

@zadjii-msft
Copy link
Member

Oh that's why my "Good Morning" script seems to hang. Not because it's asking the Terminal to do too much, but because the launch of WSL is slow.

I wonder if we can start the process maybe on a background thread? Or if we're waiting for the connection to actually start before doing UI work, could we just, not?

@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Help Wanted We encourage anyone to jump in on these. labels Mar 22, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Mar 22, 2021
@zadjii-msft zadjii-msft added this to the Terminal v2.0 milestone Mar 22, 2021
@DHowett
Copy link
Member

DHowett commented Mar 22, 2021

@zadjii-msft not without risking the WSL profiles (and if one is set as default, a pop-up dialog saying "lol you messed up somehow").

This is, in part, why I keep talking about caching generated profiles 😉

@zaxbux
Copy link
Author

zaxbux commented Mar 22, 2021

@DHowet I ended up nuking my WSL distros, and the issue went away. Watchdog of some sort would be nice, since the only way I could kill the terminal process was though Terminal's PC Settings "Terminate" button.

@DHowett
Copy link
Member

DHowett commented Mar 24, 2021

Oh, I realize now @zadjii-msft that you're talking about spawning the connection in the background, not launching WSL to scan for things.

We also evaluate the starting directory for validity (!) during settings load.

@zadjii-msft
Copy link
Member

Ohkay good, I thought there must have been a miscommunication.

We also evaluate the starting directory for validity (!) during settings load.

Oh well that's probably what's doing it. Maybe we should just ditch that 🤔

@zadjii-msft
Copy link
Member

I'm bumping this to 1.8 - we might want to preemptively fix this, esp. before pushing OSC9;9 to stable

@zadjii-msft zadjii-msft self-assigned this Apr 7, 2021
@zadjii-msft zadjii-msft added Priority-1 A description (P1) Severity-Blocking We won't ship a release like this! No-siree. and removed Help Wanted We encourage anyone to jump in on these. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Priority-2 A description (P2) labels Apr 7, 2021
@zadjii-msft
Copy link
Member

zadjii-msft commented Apr 7, 2021

Oh no, this isn't what I thought it was. I thought we had some quick path validation if a path existed for the OSC9;9 case. But we actually don't...

EDIT:

Ah no, the line in question is in TerminalSettings::_ApplyProfileSettings

        _StartingDirectory = profile.EvaluatedStartingDirectory();
std::wstring Profile::EvaluateStartingDirectory(const std::wstring& directory)
{
    // First expand path
    DWORD numCharsInput = ExpandEnvironmentStrings(directory.c_str(), nullptr, 0);
    std::unique_ptr<wchar_t[]> evaluatedPath = std::make_unique<wchar_t[]>(numCharsInput);
    THROW_LAST_ERROR_IF(0 == ExpandEnvironmentStrings(directory.c_str(), evaluatedPath.get(), numCharsInput));

    // Validate that the resulting path is legitimate
    const DWORD dwFileAttributes = GetFileAttributes(evaluatedPath.get());
    ...

WHICH HAS LITERALLY ALWAYS EXISTED:
https://github.com/microsoft/terminal/blame/ed1cd32f1fb0c681b05998efdee124d6c314a07d/src/cascadia/TerminalSettingsModel/Profile.cpp#L407

LITERALLY SINCE d4d59fa

So I'm punting this back out of 1.8.

@zadjii-msft zadjii-msft removed Priority-1 A description (P1) Severity-Blocking We won't ship a release like this! No-siree. labels Apr 7, 2021
@zadjii-msft zadjii-msft added the Priority-2 A description (P2) label Apr 7, 2021
@zadjii-msft zadjii-msft removed their assignment Apr 7, 2021
@stevenbrix
Copy link

for the record, i'm not seeing this issue with it hanging, i used the path:

\\wsl.localhost\Ubuntu\home\stevenki

windows ver: 21364.1
terminal ver: 1.7.1033.0

zadjii-msft added a commit that referenced this issue May 6, 2021
This is promarily being done to unblock #9223.

Prior to this, we'd validate that the user's `startingDirectory` existed
here. If it was invalid, we'd gracefully fall back to `%USERPROFILE%`.

However, that could cause hangs when combined with WSL. When the WSL
filesystem is slow to respond, we'll end up waiting indefinitely for
their filesystem driver to respond. This can result in the whole terminal
becoming unresponsive.

Similarly, with #9223 we want users to be able to specify WSL paths in a
profile, but this bit of validation logic totally prevents that from working,
because it'll just replace the path with `%USERPROFILE%`.

If the path is eventually invalid, we'll display warning in the
`ConptyConnection`, when the process fails to launch.

Closes #9541
Closes #9114
@ghost ghost added the In-PR This issue has a related PR label May 6, 2021
@ghost ghost closed this as completed in #10045 May 12, 2021
@ghost ghost removed the In-PR This issue has a related PR label May 12, 2021
ghost pushed a commit that referenced this issue May 12, 2021
This is primarily being done to unblock #9223.

Prior to this, we'd validate that the user's `startingDirectory` existed
here. If it was invalid, we'd gracefully fall back to `%USERPROFILE%`.

However, that could cause hangs when combined with WSL. When the WSL
filesystem is slow to respond, we'll end up waiting indefinitely for
their filesystem driver to respond. This can result in the whole terminal
becoming unresponsive.

Similarly, with #9223 we want users to be able to specify WSL paths in a
profile, but this bit of validation logic totally prevents that from working,
because it'll just replace the path with `%USERPROFILE%`.

If the path is eventually invalid, we'll display warning in the
`ConptyConnection`, when the process fails to launch.

Closes #9541
Closes #9114

![image](https://user-images.githubusercontent.com/18356694/117318675-426d2d00-ae50-11eb-9cc0-0b23c397472c.png)
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label May 12, 2021
DHowett pushed a commit that referenced this issue May 24, 2021
This is primarily being done to unblock #9223.

Prior to this, we'd validate that the user's `startingDirectory` existed
here. If it was invalid, we'd gracefully fall back to `%USERPROFILE%`.

However, that could cause hangs when combined with WSL. When the WSL
filesystem is slow to respond, we'll end up waiting indefinitely for
their filesystem driver to respond. This can result in the whole terminal
becoming unresponsive.

Similarly, with #9223 we want users to be able to specify WSL paths in a
profile, but this bit of validation logic totally prevents that from working,
because it'll just replace the path with `%USERPROFILE%`.

If the path is eventually invalid, we'll display warning in the
`ConptyConnection`, when the process fails to launch.

Closes #9541
Closes #9114

![image](https://user-images.githubusercontent.com/18356694/117318675-426d2d00-ae50-11eb-9cc0-0b23c397472c.png)

(cherry picked from commit bfc4838)
@ghost
Copy link

ghost commented May 25, 2021

🎉This issue was addressed in #10045, which has now been successfully released as Windows Terminal v1.8.1444.0.:tada:

Handy links:

@ghost
Copy link

ghost commented May 25, 2021

🎉This issue was addressed in #10045, which has now been successfully released as Windows Terminal Preview v1.9.1445.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants