starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-04 05:35:20 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

We need to do something about these feature tests #15294

New Issue
Open
opened 2026-01-31 04:34:04 +00:00 by claunia · 5 comments
claunia commented 2026-01-31 04:34:04 +00:00
Owner
Copy Link

Originally created by @zadjii-msft on GitHub (Sep 20, 2021).

Seriously. I'd reckon that 50% of our CI runs fail due to this. Dunno what happened to make these flakier. EIM seems like a good time to address this.

Previous notes:

We're definitely correctly waiting on the conhost / openconsole process to start. It's just that some percent of the time, it still just fails to re-open the stdout handle and attach to that console. Crazy.

Originally created by @zadjii-msft on GitHub (Sep 20, 2021). Seriously. I'd reckon that 50% of our CI runs fail due to this. Dunno what happened to make these flakier. EIM seems like a good time to address this. Previous notes: * #11858 * #8534 * #15106 We're definitely correctly waiting on the conhost / openconsole process to start. It's just that some percent of the time, it still just fails to re-open the stdout handle and attach to that console. Crazy.
claunia added the Issue-TaskProduct-MetaArea-Build labels 2026-01-31 04:34:04 +00:00
claunia commented 2026-01-31 04:34:05 +00:00
Author
Owner
Copy Link

@tusharsnx commented on GitHub (Aug 27, 2023):

I spend some time digging into this.

We're definitely correctly waiting on the conhost / openconsole process to start.

I can confirm this. Every call to freopen_s is reaching the newly spawned openconsole properly. It invokes two calls inside openconsole, first CONSOLE_IO_CLOSE_OBJECT and then CONSOLE_IO_CREATE_OBJECT inside IoSorter.cpp.

@zadjii-msft We can't see what raw ioctl reply is received by the application, can we? I'm assuming this is implemented within ConsoleApi*.h which is not part of this repository.

@tusharsnx commented on GitHub (Aug 27, 2023): I spend some time digging into this. > We're definitely correctly waiting on the conhost / openconsole process to start. I can confirm this. Every call to `freopen_s` is reaching the newly spawned `openconsole` properly. It invokes two calls inside openconsole, first `CONSOLE_IO_CLOSE_OBJECT` and then `CONSOLE_IO_CREATE_OBJECT` inside `IoSorter.cpp`. @zadjii-msft We can't see what raw ioctl reply is received by the application, can we? I'm assuming this is implemented within `ConsoleApi*.h` which is not part of this repository.
claunia commented 2026-01-31 04:34:05 +00:00
Author
Owner
Copy Link

@tusharsnx commented on GitHub (Aug 27, 2023):

More details:

Even the first call to freopen_s within the sleep (for) loop makes it to the newly spawned console, which indicate that sleep shouldn't be required for 'wait for console to get ready'.

It just that the new handles that were created/returned by the conhost are sometimes valid, sometimes not, and sometimes never even after trying several times. Seems like a magic so far 🤔.

Removing these lines is enough to stop tests from failing 100% of the time out of my 30 min of testing it with 'run until failure' (though I now don't know how to stop a test that passes all the time 😂, had to close VS)

821ae3af2d/src/host/ft_host/InitTests.cpp (L256-L259)

We could get rid of the whole sleep loop, since I tested with/without it and it worked regardless.

This led me to believe that our ObjectHandleData create/destroy is busted somewhere, surety level: negative.

If someone could confirm these, I'd be happy to raise a PR proposing these changes.

@tusharsnx commented on GitHub (Aug 27, 2023): More details: Even the first call to `freopen_s` within the sleep (for) loop makes it to the newly spawned console, which indicate that sleep shouldn't be required for 'wait for console to get ready'. It just that the new handles that were created/returned by the conhost are sometimes valid, sometimes not, and sometimes never even after trying several times. Seems like a magic so far 🤔. Removing these lines is enough to stop tests from failing 100% of the time out of my 30 min of testing it with 'run until failure' (though I now don't know how to stop a test that passes all the time 😂, had to close VS) https://github.com/microsoft/terminal/blob/821ae3af2d311350fbfaa4c77af09858488681e9/src/host/ft_host/InitTests.cpp#L256-L259 *We could get rid of the whole sleep loop, since I tested with/without it and it worked regardless.* This led me to believe that our `ObjectHandleData` create/destroy is busted somewhere, surety level: negative. If someone could confirm these, I'd be happy to raise a PR proposing these changes.
claunia commented 2026-01-31 04:34:05 +00:00
Author
Owner
Copy Link

@lhecker commented on GitHub (Aug 28, 2023):

We could get rid of the whole sleep loop, since I tested with/without it and it worked regardless.

Nice finding!

Getting rid of the freopen_s will be hard, because we use the CRT printing functions (like printf) in some tests. IMO we could just remove those tests because they test the CRT's and not conhost's correctness.
Alternatively, we could use _open_osfhandle and _fdopen to turn the new console handles into new FILE* handles without the need to close or reopen the old ones.

@lhecker commented on GitHub (Aug 28, 2023): > We could get rid of the whole sleep loop, since I tested with/without it and it worked regardless. Nice finding! Getting rid of the `freopen_s` will be hard, because we use the CRT printing functions (like `printf`) in some tests. IMO we could just remove those tests because they test the CRT's and not conhost's correctness. Alternatively, we could use `_open_osfhandle` and `_fdopen` to turn the new console handles into new `FILE*` handles without the need to close or reopen the old ones.
claunia commented 2026-01-31 04:34:06 +00:00
Author
Owner
Copy Link

@tusharsnx commented on GitHub (Aug 28, 2023):

One more thing. A huge one.

The testhost surprisingly is not a console app, so the new handle that is created is not set as a standard input/output handle by ucrt, and is not a standard handle. Whatever GetStdHandle returns is old and very well a wrong handle. I'm sure we didn't expect that 😅.

We can easily workaround that by doing SetStdHandle(STD_INPUT_HANDLE, _get_osfhandle(_fileno(stdin)))

Getting rid of the freopen_s will be hard, because we use the CRT printing functions (like printf) in some tests.

Didn't notice that, thanks for the heads up. 👍

@tusharsnx commented on GitHub (Aug 28, 2023): One more thing. A huge one. The testhost surprisingly is *not a console app*, so the new handle that is created is not set as a standard input/output handle by ucrt, and is not a standard handle. Whatever GetStdHandle returns is old and very well a wrong handle. I'm sure we didn't expect that 😅. We can easily workaround that by doing `SetStdHandle(STD_INPUT_HANDLE, _get_osfhandle(_fileno(stdin)))` > Getting rid of the freopen_s will be hard, because we use the CRT printing functions (like printf) in some tests. Didn't notice that, thanks for the heads up. 👍
claunia commented 2026-01-31 04:34:06 +00:00
Author
Owner
Copy Link

@tusharsnx commented on GitHub (Aug 29, 2023):

Just to give an idea, here is where it checks if SetStdHandle needs to be called for the new handle.

In ucrt/lowio/osfinfo.cpp:

Screenshot 2023-08-28 181335

_query_app_type() returns _crt_app_unknown and skips the if body.

@tusharsnx commented on GitHub (Aug 29, 2023): Just to give an idea, here is where it checks if SetStdHandle needs to be called for the new handle. In `ucrt/lowio/osfinfo.cpp`: ![Screenshot 2023-08-28 181335](https://github.com/microsoft/terminal/assets/55626797/ac152e96-4a59-4fbc-b6a1-476878e6dc29) `_query_app_type()` returns `_crt_app_unknown` and skips the `if` body.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label Area-Build Issue-Task Product-Meta
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#15294
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?