ConPTY: Flush unhandled sequences to the buffer by lhecker · Pull Request #17741 · microsoft/terminal

lhecker · 2024-08-19T17:30:19Z

#17510 made it so that VT requests like DA1 are passed through to the
hosting terminal and so conhost stopped responding to them on its own.
But since our input parser doesn't support proper passthrough (yet),
it swallowed the response coming from the terminal.

To solve this issue, this PR repurposes the existing boolean return
values to indicate to the parser whether the current sequence should
be flushed to the dispatcher as-is. The output parser always returns
true (success) and leaves its pass-through handler empty, while the
input parser returns false for sequences it doesn't expect.

Validation Steps Performed

Launch cmd
Press Ctrl+[, [, c, Enter (= ^[[c = DA1 request)
DA1 response is visible ✅

lhecker · 2024-08-19T17:33:00Z

src/terminal/parser/stateMachine.cpp

+    if (!ok)
+    {
+        FlushToTerminal();
+    }


This is the important bit.

Should we flush it even if it made us throw an exception?

I think that's an edge case that's not too important, at least initially. There shouldn't be any exceptions unless it's an OOM or similar exceptional issue. Failure to parse a sequence should never raise an exception IMO.

DHowett

Largely mechanical, but this does change how passed-through VT input is encoded (from only keydown events with no scancode to full key events as though a human typed them on a keyboard), and I want to know whether that's scary.

You also got rid of most of the users of RETURN_BOOL_IF_FALSE, which is a macro of our own creation. Want to go the rest of the way and kill it, too?

src/terminal/adapter/InteractDispatch.cpp

src/terminal/parser/InputStateMachineEngine.cpp

DHowett · 2024-08-20T00:37:19Z

src/terminal/parser/stateMachine.cpp

+    if (!ok)
+    {
+        FlushToTerminal();
+    }


Should we flush it even if it made us throw an exception?

DHowett · 2024-08-20T00:39:44Z

(I'd love to see a bit more validation on WSL as well, and I suspect @j4james will have some feelings about this! I'm in favor of the architectural shift.)

j4james

I still need to look at the InputStateMachine. Just wanted to get the return value refactoring out of the way first. Btw, I'm very happy with this change - I was actually about to propose doing it myself.

src/terminal/adapter/ut_adapter/adapterTest.cpp

src/terminal/adapter/adaptDispatch.cpp

src/terminal/parser/OutputStateMachineEngine.cpp

lhecker · 2024-08-20T15:00:49Z

Thank you so much for the exhaustive review @j4james!

lhecker · 2024-08-20T20:05:12Z

Largely mechanical, but this does change how passed-through VT input is encoded (from only keydown events with no scancode to full key events as though a human typed them on a keyboard), and I want to know whether that's scary.

FYI I reverted that bit.

You also got rid of most of the users of RETURN_BOOL_IF_FALSE, which is a macro of our own creation. Want to go the rest of the way and kill it, too?

✅

(I'd love to see a bit more validation on WSL as well, and I suspect j4james will have some feelings about this! I'm in favor of the architectural shift.)

I'm still somewhat squirmish about the change, as the bool returns were used in tests. On the other hand, I feel like there's better ways to test our parser than via bool returns (e.g. by testing side effects).

I've tried reproducing any issues under WSL and couldn't find anything immediately obvious. It seemed fine with various key combinations and in vttest.

DHowett

Let's go wild. We're already blowing up the world by removing conpty, so let's just go all-in.

j4james · 2024-08-20T20:22:31Z

I've been doing some testing of this branch, and there are still a bunch of queries that don't seem to work at the cmd prompt which previously used to work

echo ^[[6n (should return a CPR report, but now generates an F3 key press instead)
echo ^[[=c (should return a DA3 report like ^[P!|00000000^[\, but we now just get P!|\).
echo ^[[18t (should return a size report like ^[[8;24;80t, but we now get nothing).

There are many more, but I think they are all just variations of the above 3 cases.

I'm also now seeing some issues with reports in a WSL shell, which I'm almost sure were working before. I think it's all just DCS reports. For example, printf "\e[=c"; read should produce a report like ^[P!|00000000^[\, but now we get something like ^[P!|^[P!|00000000^[\.

src/terminal/parser/stateMachine.cpp

src/terminal/parser/InputStateMachineEngine.cpp

lhecker · 2024-08-21T13:57:37Z

echo ^[[6n: I solved this by checking for _encounteredWin32InputModeSequence. I'm not sure if there's a better alternative, except for tracking DSR in the output parser. That seemed like a heuristic to me that's just as bad, since input/output run concurrent to each other.

echo ^[[=c: In cmd, this parses into 0 parameters and runs into ActionEscDispatch and so it gets treated completely incorrectly. I don't understand why that happens.

DHowett · 2024-08-21T15:11:45Z

src/terminal/parser/InputStateMachineEngine.cpp

        _WriteSingleKey(VK_TAB, SHIFT_PRESSED);
        return true;
-    case CsiActionCodes::DTTERM_WindowManipulation:
-        _pDispatch->WindowManipulation(parameters.at(0), parameters.at(1), parameters.at(2));


Does this mean we can unimplement InteractDispatch::WindowManipulation as well?

It's still used in OutputStateMachineEngine isn't it?

Right, but that goes through adaptDispatch.

The one from ISM goes through InteractDispatch.

And upon looking at InteractDispatch::WindowManipulation, I fear that we actually might be using it (window manipulation) to help conpty's window transition through states ......

…put-flush-unhandled

zadjii-msft

this is really just like 6 lines of meat and 1200 lines of meh

zadjii-msft · 2024-08-22T22:05:51Z

src/terminal/parser/stateMachine.cpp

@@ -728,14 +726,14 @@ void StateMachine::_ActionDcsDispatch(const wchar_t wch)

    const auto success = _SafeExecute([=]() {


So this lambda, combined with "the important bit" below, is like, the actual PR here?

Yeah. I apologize for the complete mess of this PR. ☹️
Initially I thought this would be easy, but then the sunken cost fallacy hit me. ☹️☹️

ConPTY: Flush unhandled sequences to the buffer

3b9fc7e

lhecker added Product-Conpty For console issues specifically related to conpty Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) labels Aug 19, 2024

lhecker commented Aug 19, 2024

View reviewed changes

DHowett requested changes Aug 20, 2024

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 20, 2024

Fix build failure, Address feedback

88b2eed

microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 20, 2024

j4james reviewed Aug 20, 2024

View reviewed changes

lhecker added 3 commits August 20, 2024 16:51

Address feedback, Fix tests

9a2e5ae

Fix AuditMode failure

2410bde

Remove unnecessary test

c55175b

This doesn't fail locally hmm

c290bbb

DHowett approved these changes Aug 20, 2024

View reviewed changes

j4james reviewed Aug 21, 2024

View reviewed changes

src/terminal/parser/stateMachine.cpp Show resolved Hide resolved

j4james reviewed Aug 21, 2024

View reviewed changes

src/terminal/parser/InputStateMachineEngine.cpp Outdated Show resolved Hide resolved

Fix CPR, DA3 in WSL, xterm report 8, Assert bool

155baa5

DHowett reviewed Aug 21, 2024

View reviewed changes

lhecker added 2 commits August 22, 2024 00:06

Merge branch 'main' into dev/lhecker/conpty-input-flush-unhandled

67690bd

Merge remote-tracking branch 'origin/main' into dev/lhecker/conpty-in…

d7d313c

…put-flush-unhandled

zadjii-msft added this to the Terminal v1.22 milestone Aug 22, 2024

zadjii-msft approved these changes Aug 22, 2024

View reviewed changes

DHowett merged commit 6dd9c46 into main Aug 22, 2024

DHowett deleted the dev/lhecker/conpty-input-flush-unhandled branch August 22, 2024 22:52

		@@ -728,14 +726,14 @@ void StateMachine::_ActionDcsDispatch(const wchar_t wch)

		const auto success = _SafeExecute([=]() {

Conversation

lhecker commented Aug 19, 2024

Validation Steps Performed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett commented Aug 20, 2024

Uh oh!

j4james left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lhecker commented Aug 20, 2024

Uh oh!

lhecker commented Aug 20, 2024

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

j4james commented Aug 20, 2024

Uh oh!

Uh oh!

Uh oh!

lhecker commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lhecker Aug 20, 2024 •

edited

Loading

lhecker commented Aug 21, 2024 •

edited

Loading