fix(query): don't query the terminal on handles that are not terminals#713
Open
willemkokke wants to merge 1 commit into
Open
fix(query): don't query the terminal on handles that are not terminals#713willemkokke wants to merge 1 commit into
willemkokke wants to merge 1 commit into
Conversation
On Windows, BackgroundColor replaced any handle that was not a terminal with CONIN$/CONOUT$ and queried the console instead. A program whose stdio is redirected -- a Windows service, a CI job -- therefore wrote an OSC 11 query to a console nobody is watching and then waited for a reply that never came. Unix already returns an error when the handles are not terminals. Do the same on Windows: the caller passed those handles, so honour them. A caller that really wants to talk to the console can open CONIN$/CONOUT$ and pass them in, which the API already allows. Worth flagging separately: the compat package runs this query from package-level vars, so it fires before main() for anything that imports it, even to print a version string.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while tracking down a start-up hang in tea, the Gitea CLI,
which blocks forever on Windows when its stdio is redirected: https://gitea.com/gitea/tea/issues/1053
The problem
On Windows,
BackgroundColorreplaces any handle that is not a terminal withCONIN$/CONOUT$and queries the console instead:
So a program whose stdio is redirected — a Windows service, a CI job — writes an OSC 11 query to a
console nobody is watching, and then waits for a reply that never comes. Redirecting stdio does not
protect you, which is surprising: on Unix the same function returns
input/output is not a terminaland gives up.
There is a 2 second timeout on the read, but it does not currently fire, because the
CONIN$handlethis code creates defeats ultraviolet's cancel reader. That is a separate bug and I have opened
charmbracelet/ultraviolet#138 for it. With that fixed, the behaviour here becomes a 2 second stall per call instead of
a hang — better, but still wrong for a program that is only trying to print a version string.
The fix
Do what Unix already does: if the handles the caller passed are not terminals, say so and return.
The caller chose those handles. If output is going to a pipe, the console's background colour is
not the question being asked, and adaptive colour has nothing to adapt to. A caller that genuinely
wants to talk to the console can open
CONIN$/CONOUT$and pass them in — the API takes explicithandles, so that option is still there.
The Windows branch stays, because console input and output are separate handles and must be queried
as a pair rather than treating either one as bidirectional.
I realise this deletes a deliberate special case, so I am happy to be told the intent was something
I have missed. If the redirected-stdio path needs to stay, the alternative is to keep it and rely on
the ultraviolet fix to bound it — but then every non-interactive Windows program pays a 2 second
stall on start-up, and via the
compatpackage it pays it beforemain()runs.Tests
query_test.go, cross-platform, usingos.Pipe()handles:BackgroundColormust report that pipes are not terminals, and return rather than waitHasDarkBackgroundmust fall back to its documented dark defaultThe first fails against
mainon Windows — it returns no error, because it went and queried theconsole anyway. Both pass with the fix. Full suite passes.
Unrelated, but worth flagging
compatruns this query from package-level vars:Importing that package for a type —
compat.AdaptiveColor— therefore makes a program query theterminal before
main()runs. That is how tea ended up hanging ontea --version. It cannot bemade lazy without changing the exported
boolto a function, so I have not touched it here, but itseems worth a look.