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.

Sign up for GitHub

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

Jump to bottom

Fix opening the debug tap #11445

Merged
merged 2 commits into from
Oct 6, 2021
Merged

Fix opening the debug tap #11445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
403be88
this fixes the crash, need commetns
zadjii-msft Oct 6, 2021
5c77f69
comments
zadjii-msft Oct 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/cascadia/TerminalApp/DebugTapConnection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,22 @@ namespace winrt::Microsoft::TerminalApp::implementation
}
void Initialize(const Windows::Foundation::Collections::ValueSet& /*settings*/) {}
~DebugInputTapConnection() = default;
void Start()
winrt::fire_and_forget Start()
{
// GH#11282: It's possible that we're about to be started, _before_
// our paired connection is started. Both will get Start()'ed when
// their owning TermControl is finally laid out. However, if we're
// started first, then we'll immediately start printing to the other
// control as well, which might not have initialized yet. If we do
// that, we'll explode.
Copy link
Member

Choose a reason for hiding this comment

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

Should we think about making it so that's not a problem? Should TermControl buffer so Connections (eventually, 3p connections) don't need to "slow down"?

Copy link
Member

Choose a reason for hiding this comment

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

I'm worried that TC calls Start before it is really "ready" to receive text honestly..

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, TC is ready for the text. The issue is that in this case, there's a connection that needs two different TC's to be ready. So this is about syncing up those two.

I considered having TC start by buffering the output internally, but I figured the options were:

  • do that which might take a few days and miss 1.12 (and mean debugtap won't be usable for debugging in 1.12)
  • do this and get it working for 1.12

Copy link
Member

Choose a reason for hiding this comment

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

Oh dang, you're right. Sorry. Yeah.

I wonder, then, whether DebugTapConn should buffer. In the future. Thanks!

//
// Instead, wait here until the other connection is started too,
// before actually starting the connection to the client app. This
// will ensure both controls are initialized before the client app
// is.
co_await winrt::resume_background();
_pairedTap->_start.wait();

_wrappedConnection.Start();
}
void WriteInput(hstring const& data)
Expand Down Expand Up @@ -59,6 +73,9 @@ namespace winrt::Microsoft::TerminalApp::implementation
void DebugTapConnection::Start()
{
// presume the wrapped connection is started.

// This is explained in the comment for GH#11282 above.
_start.count_down();
}

void DebugTapConnection::WriteInput(hstring const& data)
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalApp/DebugTapConnection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include <winrt/Microsoft.Terminal.TerminalConnection.h>
#include "../../inc/cppwinrt_utils.h"
#include <til/latch.h>

namespace winrt::Microsoft::TerminalApp::implementation
{
Expand Down Expand Up @@ -36,6 +37,8 @@ namespace winrt::Microsoft::TerminalApp::implementation
winrt::weak_ref<Microsoft::Terminal::TerminalConnection::ITerminalConnection> _wrappedConnection;
winrt::weak_ref<Microsoft::Terminal::TerminalConnection::ITerminalConnection> _inputSide;

til::latch _start{ 1 };

friend class DebugInputTapConnection;
};
}
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/ControlCore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1025,7 +1025,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation

TerminalConnection::ConnectionState ControlCore::ConnectionState() const
{
return _connection.State();
return _connection ? _connection.State() : TerminalConnection::ConnectionState::Closed;
Copy link
Member

Choose a reason for hiding this comment

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

Prefer NotConnected here -- technically (poorly documented) connections should only move one direction (NotConn->Connecting->Connected->Closing->Closed or ...->Failed (at any point))

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 I actually hit in teardown, weirdly enough. A control closed, which bubbled up and out to the pane, who asked the control what its ConnectionState() was, but the _connection had already been swapped with nullptr in the teardown. So I figured the connection was "closed" at this point

Copy link
Member

Choose a reason for hiding this comment

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

ty, sorry, not fully paged in 😄

}

hstring ControlCore::Title()
Expand Down