Rework locking and eventing during startup and shutdown to alleviate some VT issues by miniksa · Pull Request #2525 · microsoft/terminal

miniksa · 2019-08-23T23:42:24Z

Summary of the Pull Request

Adjusts the startup and shutdown behavior of most threads in the console host to alleviate race conditions that are either exacerbated or introduced by the VT PTY threads.

PR Checklist

Closes ClosePseudoConsole API hanging #1810
I'm an employee
Tests added/passed
Requires documentation to be updated - Please see and review Propose some documentation updates for PseudoConsole hang conditions. MicrosoftDocs/Console-Docs#100
I'm a core contributor

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

…ring shutdown activities.

…Remove extraneous attempts at cleanup.

…igure out why everything else doesn't.

…t leaves such that we can clean everything up appropriately.

…ge to tell the input thread it is allowed to operate under the lock the IO thread is holding. Without this, the signal thread breaking and shutting down may be able to acquire the lock in a split second and cause unexpected oddities to occur for shutting down console context while it is being allocated for a client.

miniksa · 2019-08-26T18:06:15Z

Please see MicrosoftDocs/Console-Docs#100 for my proposal on documentation updates.

…so they don't hang forever.

… the main IO thread because it's untenable to pass the lock in that way.

…o it can unblock them by opening thread and we're good.

…s that tear down process cleanly when they are destroyed.

miniksa · 2019-08-28T21:15:35Z

OK. I need to adapt the little test utility into a feature test for this to ensure that it remains working and stable into the future and then this will be good to go.

…urcing it from Dustin's sample utility that was used to help figure this out.

…ually be validated.

…ose what is going wrong with *some* of the permutations of this test in the CI.

miniksa · 2019-09-10T23:42:34Z

Table of the most recent failure run (since run data is trimmed over time).

metadata	endSession	inheritCursor	readOutput	writeInput
2	2	T	T	T
6	0	T	F	T
11	2	F	F	T
16	1	F	T	F
20	2	T	F	F

I updated the test to provide more verbose logging of what it is doing while it runs so I can maybe figure out where exactly it's hanging.

The table above is the runs that failed. I can't tell if it's always these 5 or if those are a random 5 because the previous run data was pruned.

On my local machine, I cannot seem to reproduce this issue. So I'm trying to come up with a way to figure out what's going wrong in the lab. The error code given is 0xc0000374 which is STATUS_HEAP_CORRUPTION. But I would think that if I screwed something up about allocations and freeing in the test to cause corruption that perhaps it would be in all inheritCursor = TRUE tests or something, not what looks like a random 5.

We'll see what happens in this CI run and I'll keep looking tomorrow.

miniksa · 2019-09-11T17:41:27Z

OK current CI run failures:

4, 7, 13, 16, 18, 22. Same error code. So it's not specific tests... its something about the tests overall.

miniksa · 2019-09-11T20:25:10Z

OK. Isolating the runs resolves this issue. This is now ready for review.

zadjii-msft

I mostly just have questions and nits

src/renderer/vt/state.cpp

src/host/globals.h

src/host/ft_host/API_PtyTests.cpp

src/host/VtInputThread.cpp

…re per PR feedback. Also removed a hungarian.

src/host/VtInputThread.cpp

src/host/VtIo.cpp

src/cascadia/ut_app/TerminalApp.Unit.Tests.manifest

zadjii-msft

👍

src/host/VtInputThread.cpp

src/host/VtIo.cpp

src/host/ft_host/API_PtyTests.cpp

src/host/output.cpp

src/renderer/vt/state.cpp

src/host/PtySignalInputThread.cpp

src/inc/ITerminalOwner.hpp

…e comments and minor nit cleanup.

…some VT issues (#2525) Adjusts the startup and shutdown behavior of most threads in the console host to alleviate race conditions that are either exacerbated or introduced by the VT PTY threads. (cherry picked from commit 56c3594)

…leviate some VT issues (#2525)" This reverts commit 56c3594. Also patches up some build fixes made after it and corrects a VtRendererTest that was dependent on the locks.

miniksa added 8 commits August 19, 2019 15:23

try to acquire the lock and try to stop synchronous waits when trigge…

e1e9961

…ring shutdown activities.

get the actual thread handle and try to cancel it.

4f2db78

rearrange the way that shutdown works completely for the VT threads. …

2128268

…Remove extraneous attempts at cleanup.

We've reached a point where the VT stuff unspools just fine. Now to f…

7f627e1

…igure out why everything else doesn't.

Add a series of events and functions that unspool when the last clien…

e136314

…t leaves such that we can clean everything up appropriately.

Fix build failures because I didn't give the event to the unit tests.

9021a5a

code format run.

3b76cc4

miniksa added 8 commits August 26, 2019 13:58

signal on destruct. add the eventing appropriately to the unit tests …

767ceab

…so they don't hang forever.

Fix to manual reset events, try to debug.

4bb890a

Merge branch 'master' into dev/miniksa/1810_attempt4

90ce9d4

Restore the ordering to causing the VT input messages to be pumped on…

562837e

… the main IO thread because it's untenable to pass the lock in that way.

Alright, make the VT input thread store and remember who is blocked s…

8e79f25

…o it can unblock them by opening thread and we're good.

oops I messed up the real input thread.

ee960dd

Fix termination occuring during test because VT threads have watchdog…

9a0cb51

…s that tear down process cleanly when they are destroyed.

Minor code formatting fixes.

dc7c888

miniksa added 6 commits August 28, 2019 16:27

Add draft of PTY init/close tests to console feature test suite by so…

81c850e

…urcing it from Dustin's sample utility that was used to help figure this out.

Merge branch 'master' into dev/miniksa/1810_attempt4

33d2759

Merge branch 'master' into dev/miniksa/1810_attempt4

d8a7d66

codeformatting pass

e1381f7

remove process leak, return the results correctly so the test can act…

aac09b3

…ually be validated.

Adding additional text output during the test to hopefully help diagn…

05a4510

…ose what is going wrong with *some* of the permutations of this test in the CI.

add isolation to attempt to resolve test issues.

6feb667

miniksa marked this pull request as ready for review September 11, 2019 20:25

miniksa requested a review from DHowett-MSFT September 11, 2019 20:25

miniksa requested review from carlos-zamora and zadjii-msft September 11, 2019 20:25

zadjii-msft reviewed Sep 12, 2019

View reviewed changes

This was referenced Sep 13, 2019

Crash on closing tab running powershell in CMD #2394

Closed

Terminal crashes on showing docker status #2434

Closed

Crash after closing an Azure tab and entering something in default PowerShell tab #2230

Closed

miniksa added 3 commits September 17, 2019 16:22

Merge branch 'master' into dev/miniksa/1810_attempt4

8018b21

Add some comments to help ensure information is captured for the futu…

d0eb967

…re per PR feedback. Also removed a hungarian.

rename apparently missed some references. correcting.

b5a32bb

DHowett-MSFT reviewed Sep 20, 2019

View reviewed changes

src/host/VtInputThread.cpp Show resolved Hide resolved

src/host/VtIo.cpp Outdated Show resolved Hide resolved

src/cascadia/ut_app/TerminalApp.Unit.Tests.manifest Outdated Show resolved Hide resolved

zadjii-msft approved these changes Sep 20, 2019

View reviewed changes

DHowett-MSFT approved these changes Sep 20, 2019

View reviewed changes

carlos-zamora reviewed Sep 20, 2019

View reviewed changes

src/host/PtySignalInputThread.cpp Show resolved Hide resolved

src/inc/ITerminalOwner.hpp Show resolved Hide resolved

miniksa added 3 commits September 20, 2019 13:53

PR feedback, a bunch related to losing things during destruction. Som…

d498d5f

…e comments and minor nit cleanup.

code formatter, you're killin' me!

9bdce5b

removing isolation didn't work, does it work now?

2a4c15e

miniksa merged commit 56c3594 into master Sep 20, 2019

miniksa deleted the dev/miniksa/1810_attempt4 branch September 20, 2019 22:43

This was referenced Oct 12, 2019

Allow FontInfo{,Base,Desired} to store a font name > 32 wch #3107

Merged

CONSOLE_INFORMATION has an ODR violation contributing to test instability #3170

Closed

zadjii-msft mentioned this pull request Oct 25, 2019

Find the actual issue keeping conhosts from exiting when tabs are closed #2185

Closed

miniksa mentioned this pull request Nov 8, 2019

Revert locking changes #3488

Merged

5 tasks

zadjii-msft mentioned this pull request Apr 20, 2020

ClosePseudoConsole API hanging #1810

Closed

Conversation

miniksa commented Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

miniksa commented Aug 26, 2019

Uh oh!

miniksa commented Aug 28, 2019

Uh oh!

miniksa commented Sep 10, 2019

Uh oh!

miniksa commented Sep 11, 2019

Uh oh!

miniksa commented Sep 11, 2019

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

miniksa commented Aug 23, 2019 •

edited

Loading