Avoid async infinite loop on tab close #15335

lhecker · 2023-05-11T23:06:27Z

When a tab gets closed, _RemoveTab will call TabBase::Shutdown(),
which then re-raises the Closed event, which will end up calling
_RemoveTab again, etc. The only reason this didn't crash WT so far
is because _RemoveOnCloseRoutine contains a resume_foreground,
which would resolve the recursion and turn it into CPU usage.
It would spin as long as WinUI hasn't discard the tab object,
which takes an unpredictable amount of time.

Raising the Closed event from Shutdown() is unnecessary, because
the handlers of the event end up calling _RemoveTab anyways.
Technically the entire Closed event can be removed now, but I left it
in anyways because resolving the architectural "knot" around the way
tab closing after the last pane closes is implemented requires much
more significant changes.

This commit additionally removes the _createCloseLock mutex in Pane
as it was very likely not working as intended anyways. Only some methods
were protected by it and it doesn't avoid any STA/MTA/NA issues either.

Validation Steps Performed

Closing tabs and panes always ends up calling Shutdown() ✅

Closes #14898

lhecker · 2023-05-11T23:09:51Z

src/cascadia/TerminalApp/TabManagement.cpp

+        if (tab == _settingsTab)
+        {
+            _settingsTab = nullptr;
+        }


I felt like this is a more robust way to express the same idea of "there's just one instance of settings and closing it destroys it".

DHowett

What about when a tab closes itself, like when the terminal connection inside it exits? That does not come in on the main dispatcher thread, therefore interacting with UI elements during the Close callback is a danger.

Are we certain that Close always comes in on the right thread?

lhecker · 2023-05-12T20:32:10Z

src/cascadia/TerminalApp/Pane.cpp

@@ -1843,135 +1842,128 @@ void Pane::_CloseChild(const bool closeFirst, const bool isDetaching)
    closedChild->_ClosedByParentHandlers();
 }

-winrt::fire_and_forget Pane::_CloseChildRoutine(const bool closeFirst)
+void Pane::_CloseChildRoutine(const bool closeFirst)


You need to view this changeset with whitespace changes suppressed. :)

lhecker · 2023-05-12T20:34:08Z

Are we certain that Close always comes in on the right thread?

You're right, it didn't. In my original PR it worked, because I also made it so that the close event gets emitted on the UI thread. I now brought those changes into this PR as well, but unfortunately it increases the diff size. It's easier to view with whitespace changes suppressed.

DHowett · 2023-05-15T19:59:39Z

src/cascadia/TerminalApp/Pane.cpp

@@ -683,8 +683,6 @@ bool Pane::SwapPanes(std::shared_ptr<Pane> first, std::shared_ptr<Pane> second)
        return false;
    }

-    std::unique_lock lock{ _createCloseLock };


Do you know what this was guarding and how we are now safe without it?

DHowett

I think I understand why this works. Thanks!

1.18 material?

DHowett · 2023-05-15T21:14:19Z

Oh, you removed it from 1.18. OK

lhecker · 2023-05-15T21:17:28Z

Yeah I think I'd like to wait with this until 1.19... There's still these comments in the code and I'd like to 110% grasp what they mean in practice and if it was a practical concern to begin with:

// It's possible that this event handler started being executed, then before
// we got the lock, another thread created another child. So our control is
// actually no longer _our_ control, and instead could be a descendant.

...because to be honest I don't think any of this worked the way we imagined it would. I'm pretty sure all of that _IsLeaf() code is unneeded and always has been.

carlos-zamora

This all seems good. The only thing I want to double check with you on is why we're removing the lock from...

SwapPanes()
_CloseTerminalRequestedHandler()
Shutdown()
_CloseChild()
_CloseChildRoutine()

I get that's the point of this PR haha, but why is it safe to do that now?

lhecker · 2023-06-22T21:45:26Z

@carlos-zamora Sorry for the late response. As far as I can tell, _ControlConnectionStateChangedHandler is the only function that is being called from a background thread. It would end up calling Close() which I assume is why the mutex was called _createCloseLock. I now added a resume_foreground there so it's been fixed (hopefully).

The reason I removed it is simply because that mutex provides a false sense of security. We can't use it to make shutdown secure, when Pane has ~50 member functions and only 6 of them are thread safe. 😅 It just doesn't work at all and never did. 😅😅

zadjii-msft · 2023-08-08T16:02:09Z

This has ✅✅, but never merged while I was out. I haven't read it yet. Should we include this in the bug bash?

…nite-loop

lhecker · 2023-08-08T16:05:56Z

Yes, please. I've merged in main just now to resolve the conflict.

…nite-loop

Well, Pane doesn't _only_ care if the connection isn't entering a terminal state. It does need to update its own state first. Regressed in #15335 Closes #16068

Well, Pane doesn't _only_ care if the connection isn't entering a terminal state. It does need to update its own state first. Regressed in #15335 Closes #16068 (cherry picked from commit 4145f18) Service-Card-Id: 90731934 Service-Version: 1.19

Avoid async infinite loop on tab close

cdc0339

lhecker added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. labels May 11, 2023

lhecker commented May 11, 2023

View reviewed changes

DHowett requested changes May 12, 2023

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 12, 2023

Bring in the remaining Pane changes

26e7e0d

microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 12, 2023

lhecker commented May 12, 2023

View reviewed changes

DHowett reviewed May 15, 2023

View reviewed changes

DHowett approved these changes May 15, 2023

View reviewed changes

carlos-zamora reviewed Jun 5, 2023

View reviewed changes

carlos-zamora approved these changes Jun 30, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into dev/lhecker/pane-infi…

a9e62f5

…nite-loop

zadjii-msft added the zBugBash-Consider label Aug 8, 2023

Merge remote-tracking branch 'origin/main' into dev/lhecker/pane-infi…

f04f1c5

…nite-loop

zadjii-msft enabled auto-merge (squash) August 15, 2023 18:03

zadjii-msft merged commit 3701d0e into main Aug 15, 2023
15 of 17 checks passed

zadjii-msft deleted the dev/lhecker/pane-infinite-loop branch August 15, 2023 19:12

zadjii-msft mentioned this pull request Sep 19, 2023

Terminal hangs when exiting a debug tap session #14898

Closed

zadjii-msft added a commit that referenced this pull request Oct 2, 2023

Fix closeOnExit: always

56625cc

Well, Pane doesn't _only_ care if the connection isn't entering a terminal state. It does need to update its own state first. Regressed in #15335 Closes #16068

zadjii-msft mentioned this pull request Oct 2, 2023

Fix closeOnExit: always #16090

Merged

DHowett pushed a commit that referenced this pull request Oct 3, 2023

Fix closeOnExit: always (#16090)

4145f18

Well, Pane doesn't _only_ care if the connection isn't entering a terminal state. It does need to update its own state first. Regressed in #15335 Closes #16068

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid async infinite loop on tab close #15335

Avoid async infinite loop on tab close #15335

lhecker commented May 11, 2023 •

edited by zadjii-msft

Loading

lhecker May 11, 2023

DHowett left a comment

lhecker May 12, 2023

lhecker commented May 12, 2023

DHowett May 15, 2023

DHowett left a comment

DHowett commented May 15, 2023

lhecker commented May 15, 2023 •

edited

Loading

carlos-zamora left a comment •

edited

Loading

lhecker commented Jun 22, 2023 •

edited

Loading

zadjii-msft commented Aug 8, 2023

lhecker commented Aug 8, 2023

Avoid async infinite loop on tab close #15335

Avoid async infinite loop on tab close #15335

Conversation

lhecker commented May 11, 2023 • edited by zadjii-msft Loading

Validation Steps Performed

lhecker May 11, 2023

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

lhecker May 12, 2023

Choose a reason for hiding this comment

lhecker commented May 12, 2023

DHowett May 15, 2023

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented May 15, 2023

lhecker commented May 15, 2023 • edited Loading

carlos-zamora left a comment • edited Loading

Choose a reason for hiding this comment

lhecker commented Jun 22, 2023 • edited Loading

zadjii-msft commented Aug 8, 2023

lhecker commented Aug 8, 2023

lhecker commented May 11, 2023 •

edited by zadjii-msft

Loading

lhecker commented May 15, 2023 •

edited

Loading

carlos-zamora left a comment •

edited

Loading

lhecker commented Jun 22, 2023 •

edited

Loading