Skip to content

Commit

Permalink
Don't wait for a connection to finish when a control is closed (#5303)
Browse files Browse the repository at this point in the history
## Summary of the Pull Request

When a pane is closed by a connection, we want to wait until the connection is actually `Closed` before we fire the actual `Closed` event. If the connection didn't close gracefully, there are scenarios where we want to print a message to the screen.

However, when a pane is closed by the UI, we don't really care to wait for the connection to be completely closed. We can just do it whenever. So I've moved that call to be on a background thread.

## PR Checklist
* [x] Closes #1996
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments
Previously we'd wait for the connection to close synchronously when closing tabs or panes. For misbehaving applications like `ssh.exe`, that could result in the `Close` needing to `WaitForSingleObject` _on the UI thread_. If the user closed the tab / pane either with a keybinding or with some other UI element, they don't really care to see the error message anymore. They just want the pane closed. So there's no need to wait for the actual connection to close - the app can just continue on with whatever it was doing.

## Validation Steps Performed
Messed around with closing tabs, panes, tabs with many panes, the entire window. Did this with keybindings, or by clicking on the 'x' on the tab, the 'x' on the window, or using middle-click.

I'm always scared of things like this, so there's a 50% chance this makes things horribly worse.
  • Loading branch information
zadjii-msft authored Apr 9, 2020
1 parent c65de31 commit 6f0f245
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 5 deletions.
31 changes: 26 additions & 5 deletions src/cascadia/TerminalControl/TermControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1970,6 +1970,26 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
_clipboardPasteHandlers(*this, *pasteArgs);
}

// Method Description:
// - Asynchronously close our connection. The Connection will likely wait
// until the attached process terminates before Close returns. If that's
// the case, we don't want to block the UI thread waiting on that process
// handle.
// Arguments:
// - <none>
// Return Value:
// - <none>
winrt::fire_and_forget TermControl::_AsyncCloseConnection()
{
if (auto localConnection{ std::exchange(_connection, nullptr) })
{
// Close the connection on the background thread.
co_await winrt::resume_background();
localConnection.Close();
// connection is destroyed.
}
}

void TermControl::Close()
{
if (!_closing.exchange(true))
Expand All @@ -1981,11 +2001,12 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
TSFInputControl().Close(); // Disconnect the TSF input control so it doesn't receive EditContext events.
_autoScrollTimer.Stop();

if (auto localConnection{ std::exchange(_connection, nullptr) })
{
localConnection.Close();
// connection is destroyed.
}
// GH#1996 - Close the connection asynchronously on a background
// thread.
// Since TermControl::Close is only ever triggered by the UI, we
// don't really care to wait for the connection to be completely
// closed. We can just do it whenever.
_AsyncCloseConnection();

if (auto localRenderEngine{ std::exchange(_renderEngine, nullptr) })
{
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalControl/TermControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
void _CurrentCursorPositionHandler(const IInspectable& sender, const CursorPositionEventArgs& eventArgs);
void _FontInfoHandler(const IInspectable& sender, const FontInfoEventArgs& eventArgs);

winrt::fire_and_forget _AsyncCloseConnection();

// this atomic is to be used as a guard against dispatching billions of coroutines for
// routine state changes that might happen millions of times a second.
// Unbounded main dispatcher use leads to massive memory leaks and intense slowdowns
Expand Down

0 comments on commit 6f0f245

Please sign in to comment.