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

Terminal doesn't close after FreeConsole() #16174

Closed
EXTREMEGABEL opened this issue Oct 14, 2023 · 14 comments
Closed

Terminal doesn't close after FreeConsole() #16174

EXTREMEGABEL opened this issue Oct 14, 2023 · 14 comments
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Repro We can't figure out how to make this happen. Please help find a simplified repro. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-External For issues that are outside this codebase

Comments

@EXTREMEGABEL
Copy link

Windows Terminal version

1.17.11461.0

Windows build number

10.0.19045.0

Other Software

ImHex 1.31.0

Steps to reproduce

Set Windows Terminal to be the standard console app
Start ImHex via explorer

Expected Behavior

As ImHex calls FreeConsole() right before launching the GUI executable, the console window should close.

This works with the "standard" console host.

Actual Behavior

The Console Window stays open.
Occasionally it minimizes.

@EXTREMEGABEL EXTREMEGABEL added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Oct 14, 2023
@lhecker
Copy link
Member

lhecker commented Oct 16, 2023

I don't quite get what the purpose of the imhex.exe executable is when imhex-gui.exe also allocates a console. Now, whenever you launch imhex.exe from Explorer, it launches 2 console instances that are instantly closed. A better approach for this would be to:

  • Make imhex-gui.exe a /SUBSYSTEM:WINDOWS application
  • Launch imhex-gui.exe when imhex.exe runs, but then wait for the child process to exit
  • Pass an environment variable along, like _started_from_console (or a commandline parameter)
  • When it sees that env variable (or commandline parameter), imhex-gui.exe should call AttachConsole()

This is the same approach that mpv uses for its mpv.com wrapper:

Would you like to report this to ImHex?


The above aside, I'm unable to reproduce the issue. Does it still happen if you update to the latest Windows Terminal (1.18)?

@lhecker lhecker added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 16, 2023
@EXTREMEGABEL
Copy link
Author

Can I somehow force the app to update to 1.18? The Store won't distribute it to me. (However during dicussions with other users on the Discord someone metioned having the same Issue with WT 1.18)
I tested it with the 1.19 Preview Version and it behaves identically.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Oct 16, 2023
@zadjii-msft
Copy link
Member

You can always grab the latest releases straight off our https://github.com/microsoft/terminal/releases page. Double-click installing the .msixbundle should update the current package you have (and leave it linked to the Store, for future updates)

@zadjii-msft zadjii-msft added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Attention The core contributors need to come back around and look at this ASAP. labels Oct 16, 2023
@EXTREMEGABEL
Copy link
Author

Good to know Thanks!

WT 1.18.2822.0 behaves identical though

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Oct 16, 2023
@lhecker
Copy link
Member

lhecker commented Oct 17, 2023

Maybe I'm doing something wrong... I'm setting Windows Terminal 1.18 as my default terminal and then launch either imhex.exe or imhex-gui.exe. Neither reproes the issue for me. I'm thus marking this issue as needs-repro.

@EXTREMEGABEL When testing this, please make sure to have tried it without any extra software running. That is, applications like ExplorerPatcher, PowerToys, Winaero, Discord, etc., all of them should be disabled or shut down (check Task Manager to be sure). Just because I don't use any of these on my work machine right now and one of them might indirectly cause issues, even if it's an issue on our side.

@lhecker lhecker added Needs-Repro We can't figure out how to make this happen. Please help find a simplified repro. and removed Needs-Attention The core contributors need to come back around and look at this ASAP. labels Oct 17, 2023
@EXTREMEGABEL
Copy link
Author

Sorry for reporting something that aparrently noone here seems to be able to reproduce :D
That being said:
I tried this on my Win 11 machine (22621.2428) since that has way less other stuff running on it installed WT 1.18.2822.0 and sure enough Terminal doesn't close.

Can I put WT into some sort of advanced logging mode or something?

@tusharsnx
Copy link
Contributor

A screen recording that shows the bug might help.

@EXTREMEGABEL
Copy link
Author

Sure here you go:
https://youtu.be/HZC4QaGWMAQ

@DHowett
Copy link
Member

DHowett commented Oct 18, 2023

Thanks so much for doing this!

We had a look at the video and the code over in imhex, as well as grabbed a trace of process startup. Here's what's happening.

  1. imhex is a console subsystem application
  2. imhex closes its console handle (correctly), and the Terminal tab disappears
  3. imhex calls CreateProcess on imhex-gui.exe
  4. imhex-gui.exe is ALSO a console subsystem application
  5. Because imhex destroyed its console handle before CreateProcess, imhex-gui is allocated a new console host.
  6. The tab remains open, because imhex closed its console but did not prevent imhex-gui from spawning another console window.

This is why the repro video shows an ImHex tab that disappears before another one shows up.

C:\..\ImHex $ dumpbin /headers .\imhex-gui.exe | sls subsystem

            7.00 subsystem version
               3 subsystem (Windows CUI) <--- here

imhex-gui should not be a console subsystem application.
Also, for user ergonomics... the shortcut from Windows Explorer could point directly to the GUI version. Once the subsystem bit is fixed, this will remove the intermediate console completely.

@DHowett DHowett closed this as completed Oct 18, 2023
@DHowett DHowett added Tracking-External This bug isn't resolved, but it's following an external workitem. Resolution-External For issues that are outside this codebase and removed Tracking-External This bug isn't resolved, but it's following an external workitem. labels Oct 18, 2023
@DHowett
Copy link
Member

DHowett commented Oct 19, 2023

(Also, I love ImHex! If you file a bug over on their end, would you mind linking it here?)

@EXTREMEGABEL
Copy link
Author

Out of curiosity:

Your description of what happens implies to me that in actuality the standard Windows Console host behaves "incorrectly".

I thought they were supposed to behave identically.

@WerWolv
Copy link

WerWolv commented Oct 19, 2023

Hey, thanks for looking into this!
So what would be the best course of action for me? Is making imhex-gui.exe use the WINDOWS subsystem enough to fix the issue or is there more to it?

The reason all of this is necessary to begin with is because calling FreeConsole() in the main application causes crashes on older Windows versions because the console handles are closed improperly by it. That's why we moved that part to the forwarder and just attach to the real process's console.
Ultimately I need ImHex to work like a console application when launched from the console and like a regular gui application when run from the explorer

@lhecker
Copy link
Member

lhecker commented Oct 19, 2023

Your description of what happens implies to me that in actuality the standard Windows Console host behaves "incorrectly".

I thought they were supposed to behave identically.

I believe this difference is due to this fundamentally being a race condition. Windows Terminal makes it obvious, because it takes a lot longer to start and handle console API requests. (Edit: Or rather, I think it's because of the :GetCurrentProcessId() == processId check, which is also subject to races, since FreeConsole is asynchronous.)

So what would be the best course of action for me? Is making imhex-gui.exe use the WINDOWS subsystem enough to fix the issue or is there more to it?

Yes, I think doing that would be best.

Ultimately I need ImHex to work like a console application when launched from the console and like a regular gui application when run from the explorer

This will be very difficult to achieve on Windows, because while a /SUBSYSTEM:WINDOWS application can attach itself to an existing console via AttachConsole, on Windows it behaves as if you wrote imhex-gui & to run it asynchronously. We're tracking that issue at #7335.

My recommendation would be to do it similar to how mpv does it: #16174 (comment)
I believe that this approach is the best trade-off you can get right now on Windows, until we get to address #7335. If you'd like to deviate a bit from that, you could opt into always calling AttachConsole inside imhex-gui.exe, and not just when you see that flag. That way you could always see its terminal output if it's being run from inside a terminal, with the minor caveat that it behaves like imhex-gui &. After all, people can just call imhex.exe instead to get the proper blocking behavior.

All in all, this issue is really unfortunate IMO, because it's sort of on us to fix, but what we need to fix is the fundamental problem of /SUBSYSTEM:WINDOWS behaving weirdly inside terminals. 😟

@WerWolv
Copy link

WerWolv commented Oct 19, 2023

Thanks a lot! I did mostly the same thing that mpv does and it looks like it works fine, both using the old Conhost and the Windows Terminal :)
Here's the changes I made for future reference:
WerWolv/ImHex@7d17bfe

Yeah the underlying problem here is really just Windows automatically allocating a console for us in the CONSOLE subsystem. None of this would be necessary if that wasn't the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Repro We can't figure out how to make this happen. Please help find a simplified repro. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-External For issues that are outside this codebase
Projects
None yet
Development

No branches or pull requests

7 participants
@DHowett @lhecker @EXTREMEGABEL @WerWolv @zadjii-msft @tusharsnx and others