(A better) Refactoring of how connection restarting is handled#15240
(A better) Refactoring of how connection restarting is handled#15240
Conversation
…e-connection-restarting
…e-connection-restarting # Conflicts: # src/cascadia/TerminalApp/Pane.cpp # src/cascadia/TerminalApp/Pane.h
…e-connection-restarting
microsoft-github-policy-service
bot
added
Issue-Task
|
|
||
| // Fire off a connection state changed notification, to let our hosting | ||
| // app know that we're in a different state now. | ||
| if (newConnection.State() != _connection.State()) |
There was a problem hiding this comment.
_connection may be nullptr here.
lhecker
left a comment
lhecker
left a comment
There was a problem hiding this comment.
I also noticed Dustin's comment. But LGTM otherwise.
| if (!_IsLeaf()) | ||
| { | ||
| return; | ||
| } | ||
| _RestartTerminalRequestedHandlers(shared_from_this()); |
There was a problem hiding this comment.
That seems a little round-about. 😅
| if (replacing) | ||
| { | ||
| _connection.Start(); | ||
| } |
There was a problem hiding this comment.
(A theoretical concern:) If the new connection is already started, this will re-start it. Either, we should replace if (replacing) with if (_connection.State() != Started), or make sure that all implementations of Start() are robust against calling it multiple times. I think the former is better. If you did that, you can replace the other if (replacing) above with a simpler if (_connection) as well... I think. 🤔
| { | ||
| if (replacing) | ||
| { | ||
| _connection.Close(); |
There was a problem hiding this comment.
This isn't our job here - I know that we Start() it later because we want to maintain consistency (so that the first output gets written to the right place), but if I detach a connection and hold a reference to it i would expect it to keep running.
The dtor can handle it for us if TC was holding the last reference.
There was a problem hiding this comment.
actually, maybe we should move all Start() lifecycle ownership up into the component that GIVES us a connection.
Do we have a race on Initialized() vs Connection Start?
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
…e-connection-restarting
microsoft-github-policy-service
bot
removed
the
Needs-Author-Feedback
microsoft-github-policy-service
bot
added
Needs-Author-Feedback
3 participants
A different take on #14548.
ASK the app to restart its connection. This is a much more sensible approach, than leaving the ConnectionInfo in the core and having the core do the restart itself. That's mental.
Cleanup from #14060
Closes #14327
Obsoletes #14548