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

startingDirectory being invalid shouldn't cause silent failures #10225

Closed
randomascii opened this issue May 26, 2021 · 12 comments · Fixed by #10263
Closed

startingDirectory being invalid shouldn't cause silent failures #10225

randomascii opened this issue May 26, 2021 · 12 comments · Fixed by #10263
Assignees
Labels
Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@randomascii
Copy link

Windows Terminal version (or Windows build number)

1.8.1444.0

Other Software

No response

Steps to reproduce

Adjust the startingDirectory to the name of a directory that doesn't exist. Then launch Windows Terminal. That's it.

Expected Behavior

I was expecting an error message saying that the directory doesn't exist. This could be printed to the console or it could be a dialog. Ideally Windows Terminal would still launch since otherwise there is no clear way for me to fix this broken setting.

Actual Behavior

The Windows terminal window flashes and then closes. There is no indication of what the problem is and, unless you have memorized the path to the settings file, no way to fix the problem.

This issue can easily happen if you delete a directory or if you copy settings from one machine to another.

@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 May 26, 2021
@drkvogel
Copy link

I don't understand all these bug reports @craigloewen-msft about windows terminal, cause it works perfectly for me! Good job! Glad you fixed the wsl clock thing! The News and Interests new "feature" in the tray is broken, but that's another story... where can I file a bug report about that?

@KalleOlaviNiemitalo
Copy link

In #5214 (comment), it was mentioned that there was a plan to make Windows Terminal leave the session open if the process exits within five seconds. That would presumably cover invalid starting directories as well. However, I did not find an issue specifically for the five-second threshold feature.

@zadjii-msft
Copy link
Member

leave the session open if the process exits within five seconds.

I think that's probably a good idea. It kinda conflicts with the idea of "closeOnExit": "always" though. The user specifically said "I want the tab to always close when the client app exits". it just so happens, this is one of those scenarios where the client app closes immediately, and that means the only tab is going to go away.

So I guess the philosophical question is "does closeOnExit: always also apply to the window, when there's only one tab immediately closing on exit", and that doesn't seem too bad to me.

Okay I've talked myself into it

@zadjii-msft zadjii-msft added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels May 27, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 27, 2021
@zadjii-msft zadjii-msft added this to the Terminal Backlog milestone May 27, 2021
@zadjii-msft
Copy link
Member

notes from Teams:

"The connection couldn't even be established" (state transitions from "Connecting" to "Failed" without passing through "Connected")
.... Pane just observes the state transitions?
"Old=Connecting -> New=Failed: do not close"

@JoBrad
Copy link

JoBrad commented May 27, 2021

I have exactly the opposite behavior, and I don't like it. My closeOnExit value is set to graceful (not sure how that plays into this). I had set the starting directory for both Ubuntu and Powershell to the value ~, since this works in either shell. But it didn't actually work, so I also had the commandline value for Ubuntu set to wsl.exe ~ -d Ubuntu. After updating to the most recent version of Windows Terminal Preview, I now get the error below, and the shell won't start:

[error 0x8007010b when launching `powershell.exe'] Could not access starting directory "C:\Users\UserName\AppData\Local\Microsoft\WindowsApps\Microsoft.WindowsTerminalPreview_8wekyb3d8bbwe\~"

Setting the Startup directory to the parent process directory, or to %USERPROFILE% fixes the issue (and I still get put into my home directory, thanks to the commandline value).

But immediately exiting seems like a terrible way for the terminal to handle not finding a directory. It should at least default to the parent process directory, I think.

@DHowett
Copy link
Member

DHowett commented May 27, 2021

I had set the starting directory for both Ubuntu and Powershell to the value ~

This has never worked the way you're expecting. But, oh crap -- @zadjii-msft when we removed starting directory validation we also removed the magic fallback that substituted in USERPROFILE when it failed. I think this is good and correct, but it likely does account for a few of the on-launch "crashes" we've been seeing.

@DHowett
Copy link
Member

DHowett commented May 27, 2021

You can also just remove the startingDirectory. It didn't exist to begin with, and it eventually got an invalid value put into it.

That value will cease being invalid once #9223 lands.

@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label May 27, 2021
@DHowett DHowett self-assigned this May 27, 2021
@JoBrad
Copy link

JoBrad commented May 28, 2021

@DHowett To clarify, when I said "since this works in either shell" I meant that the command cd ~ works in Powershell Core and in Ubuntu, therefore wsl ~ -d <distro> is a good workaround for startingDirectory failing to parse it.

You can also just remove the startingDirectory.

If you clear the value in the settings UI, it's replaced with %USERPROFILE%, which is OK. I just wish it used the distro-specific user profile directory (e.g. ~ and #9223 ) 😀

@DHowett
Copy link
Member

DHowett commented May 28, 2021

Definitely, that all makes sense.

9223 is going to be really cool once we can validate it. It makes us completely ignore startingDirectory for WSL and pass it in as an argument (ala wsl ~) except that it also supports crazy stuff like /etc and /mnt/c.

In short, this will finally work:

{
	"commandline": "wsl",
	"startingDirectory": "/var/lib/samba"
}

DHowett added a commit that referenced this issue May 28, 2021
ConptyConnection has two different failure modes:

1. We failed to initialize the pseudoconsole or create the process
2. The process exited with an error code.

Until this commit, they were treated the same way: closeOnExit=always
would force the pane/tab to be destroyed. This was very bad in case 1,
where we would display a (possibly useful) error message and then
immediately close the window.

This was made even worse by the change in #10045. We removed
startingDirectory validation and promoted it to an error message (so
that we could eventually let the connection handle startingDirectory in
its own way.) This of course revealed that a number of users had set
invalid starting directories… and those users included some who set
closeOnExit to always. Boom: instant "terminal opens and crashes"¹

¹ It only looks like a crash; it's actually _technically_ functioning
properly.

Closes #10225.
@ghost ghost added the In-PR This issue has a related PR label May 28, 2021
DHowett added a commit that referenced this issue May 28, 2021
ConptyConnection has two different failure modes:

1. We failed to initialize the pseudoconsole or create the process
2. The process exited with an error code.

Until this commit, they were treated the same way: closeOnExit=always
would force the pane/tab to be destroyed. This was very bad in case 1,
where we would display a (possibly useful) error message and then
immediately close the window.

This was made even worse by the change in #10045. We removed
startingDirectory validation and promoted it to an error message (so
that we could eventually let the connection handle startingDirectory in
its own way.) This of course revealed that a number of users had set
invalid starting directories… and those users included some who set
closeOnExit to always. Boom: instant "terminal opens and crashes"¹

In this commit, we introduce detection for a connection that fails
before it's been established. When that happens, we will ignore the
user's closeOnExit mode.

¹ It only looks like a crash; it's actually _technically_ functioning
properly.

Closes #10225.
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels May 28, 2021
@Yangly0
Copy link

Yangly0 commented Jun 1, 2021

I met the same problem that it does not have access to "..\dir%Workspaces%".That's ok, and I just modify "%Workspaces%" to "/".

DHowett added a commit that referenced this issue Jun 1, 2021
ConptyConnection has two different failure modes:

1. We failed to initialize the pseudoconsole or create the process
2. The process exited with an error code.

Until this commit, they were treated the same way: closeOnExit=always
would force the pane/tab to be destroyed. This was very bad in case 1,
where we would display a (possibly useful) error message and then
immediately close the window.

This was made even worse by the change in #10045. We removed
startingDirectory validation and promoted it to an error message (so
that we could eventually let the connection handle startingDirectory in
its own way.) This of course revealed that a number of users had set
invalid starting directories… and those users included some who set
closeOnExit to always. Boom: instant "terminal opens and crashes"¹

In this commit, we introduce detection for a connection that fails
before it's been established. When that happens, we will ignore the
user's closeOnExit mode.

¹ It only looks like a crash; it's actually _technically_ functioning
properly.

Closes #10225.

(cherry picked from commit 31d78dc)
@ghost
Copy link

ghost commented Jul 14, 2021

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

Handy links:

@ghost
Copy link

ghost commented Jul 14, 2021

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements 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.

7 participants