Fix opening the debug tap #11445
Fix opening the debug tap #11445
Conversation
ghost
added
Area-TerminalControl
| @@ -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; | |||
There was a problem hiding this comment.
Prefer NotConnected here -- technically (poorly documented) connections should only move one direction (NotConn->Connecting->Connected->Closing->Closed or ...->Failed (at any point))
There was a problem hiding this comment.
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
| // 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. |
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
I'm worried that TC calls Start before it is really "ready" to receive text honestly..
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Oh dang, you're right. Sorry. Yeah.
I wonder, then, whether DebugTapConn should buffer. In the future. Thanks!
ghost
added
Needs-Author-Feedback
|
@msftbot merge this in 2 linux |
|
Hello @DHowett! I think you told me that you expect at least 2 approvals before I perform the merge, but I am not confident that I have understood you correctly. Please try rephrasing your instruction to me. |
|
Why did my fingers type that? |
|
🎉 Handy links: |
ghost
mentioned this pull request
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.
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.
Fixes #11282
Tested: Opened about 100 debug taps. They all worked.