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

Add ITerminalHandoff3 in preparation for overlapped pipes #17575

Merged
merged 4 commits into from
Jul 17, 2024

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Jul 16, 2024

Without a renderer in #17510 we cannot skip "frames" anymore.
As such, using overlapped IO becomes crucial to avoid a regression
in performance. ITerminalHandoff3 fixes this by allowing the terminal
to pick the pipes it wants, which mirrors CreatePseudoConsole
where the caller can also pick its own pipes.

Validation Steps Performed

  • Do a handoff with the dev build
  • Input/Output works ✅

@lhecker lhecker added the Product-Conpty For console issues specifically related to conpty label Jul 16, 2024
_cols{ 80 },
_inPipe{ hIn },
_outPipe{ hOut }
// Who decided that?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah

src/cascadia/TerminalConnection/ConptyConnection.cpp Outdated Show resolved Hide resolved
src/host/srvinit.cpp Outdated Show resolved Hide resolved
GetExitCodeProcess(_piClient.hProcess, &exitCode);

// Signal the closing or failure of the process.
// exitCode might be STILL_ACTIVE if a client has called FreeConsole() and
// thus caused the tab to close, even though the CLI app is still running.
const auto success = exitCode == 0 || exitCode == STILL_ACTIVE || exitCode == STATUS_CONTROL_C_EXIT;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

honestly I was actually fine w/ this change - I just thought the exitcode one was weird! sorry!!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, we can do such changes later, when they're necessary. 🙂

@@ -111,8 +111,8 @@
<com:ComInterface>
<com:ProxyStub Id="1D1852F4-ADAD-42B6-9A43-9437AAAD7717" DisplayName="OpenConsoleHandoffProxy" Path="OpenConsoleProxy.dll"/>
<com:Interface Id="E686C757-9A35-4A1C-B3CE-0BCC8B5C69F4" ProxyStubClsid="1D1852F4-ADAD-42B6-9A43-9437AAAD7717"/>
<com:Interface Id="59D55CCE-FC8A-48B4-ACE8-0A9286C6557F" ProxyStubClsid="1D1852F4-ADAD-42B6-9A43-9437AAAD7717"/> <!-- ITerminalHandoff -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we keep the old ITerminalHandoff around? Isn't that just good practice?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interface is essentially an internal currency between OpenConsole and the terminal. It doesn't need to support all interfaces. And after this PR OpenConsole will only support ITerminalHandoff3.

But Dustin "Human Wikipedia" Howett explained me how stubs work and basically per user there can only be 1 single COM proxy stub instance for Windows Terminal. So, if the Windows Terminal Canary stub gets installed and it only supported ITerminalHandoff3 then any older version of Windows Terminal that only supports ITerminalHandoff2 would not work anymore.

However, there's some mapping from interface GUIs to proxies so it may just work to remove them anyway. He said we should just try it.

Comment on lines -123 to -132
// Duplicate the handles from what we received.
// The contract with COM specifies that any HANDLEs we receive from the caller belong
// to the caller and will be freed when we leave the scope of this method.
// Making our own duplicate copy ensures they hang around in our lifetime.
THROW_IF_FAILED(_duplicateHandle(in, in));
THROW_IF_FAILED(_duplicateHandle(out, out));
THROW_IF_FAILED(_duplicateHandle(signal, signal));
THROW_IF_FAILED(_duplicateHandle(ref, ref));
THROW_IF_FAILED(_duplicateHandle(server, server));
THROW_IF_FAILED(_duplicateHandle(client, client));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 got moved to ConptyConnection::InitializeFromHandoff

@lhecker lhecker added this pull request to the merge queue Jul 16, 2024
@lhecker lhecker removed this pull request from the merge queue due to a manual request Jul 16, 2024
@DHowett DHowett enabled auto-merge July 16, 2024 22:31
@DHowett DHowett added this pull request to the merge queue Jul 16, 2024
@DHowett DHowett removed this pull request from the merge queue due to a manual request Jul 17, 2024
@lhecker lhecker merged commit 1f83146 into main Jul 17, 2024
20 checks passed
@lhecker lhecker deleted the dev/lhecker/17510-handoff3 branch July 17, 2024 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product-Conpty For console issues specifically related to conpty
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants