Skip DECPS/MIDI output on Ctrl+C/Break #14214

lhecker · 2022-10-13T20:55:31Z

Silent MIDI notes can be used to seemingly deny a user's input for long
durations (multiple minutes). This commit improves the situation by ignoring
all DECPS sequences for a second when Ctrl+C/Ctrl+Break is pressed.

Additionally it fixes a regression introduced in 666c446:
When we close a tab we need to unblock/shutdown MidiAudio early,
so that ConptyConnection::Close() can run down as fast as possible.

Validation Steps Performed

In pwsh in Windows Terminal 1.16 run while ($True) { echo "`e[3;8;3,~" }
- Ctrl+C doesn't do anything ❎
- Closing the tab doesn't do anything ❎
With these modifications in Windows Terminal:
- Ctrl+C stops the output ✅
- Closing the tab completes instantly ✅
With these modifications in OpenConsole:
- Ctrl+C stops the output ✅
- Closing the window completes instantly ✅

lhecker · 2022-10-13T21:00:21Z

src/audio/midi/MidiAudio.cpp

+    if (_skip.is_signaled())
+    {
+        return;
+    }


This is the new trick.

lhecker · 2022-10-13T21:01:19Z

src/audio/midi/MidiAudio.cpp

+    if (_hwnd != windowHandle)
    {
-        throw Microsoft::Console::VirtualTerminal::StateMachine::ShutdownException{};
+        _initialize(windowHandle);


I'll explain reason for this change below in ControlCore.

lhecker · 2022-10-13T21:01:41Z

src/cascadia/TerminalControl/ControlCore.cpp

+        if (ch == L'\x3') // Ctrl+C or Ctrl+Break
+        {
+            _handleControlC();
+        }


I wasn't sure where to put this...

Hmm... You want this behavior to be a part of the terminal handling it right? Like, if a user binds an action to ctrl+c, the action should be executed and this new behavior shouldn't happen right?

If that's the case, I suggest taking a look at ControlCore::TrySendKeyEvent(). By that point, we know that the input wasn't handled by an action so we can intercept it right before it goes to the shell (NOTE: this is where we have logic to update the selection from the keyboard when not in mark mode).

lhecker · 2022-10-13T21:01:55Z

src/cascadia/TerminalControl/ControlCore.cpp

+    {
+        if (!_midiAudioSkipTimer)
+        {
+            _midiAudioSkipTimer = _dispatcher.CreateTimer();


Is this the best way to create timers in Windows Terminal?

For now, yeah; we can audit dispatcher use later. Is it acceptable to Start() a timer multiple times? If the user really wails on ^C for example... which they assuredly will.

The documentation isn't particularly great

DispatcherQueueTimer.Start Method

Definition

Starts the timer.

However wading through the underlying code for this, the timer is canceled before it gets restarted.

lhecker · 2022-10-13T21:11:24Z

src/cascadia/TerminalControl/ControlCore.cpp

+            // Ensure Close() doesn't hang, waiting for MidiAudio to finish playing an hour long song.
+            _midiAudio.BeginSkip();


This is what required larger changes to MidiAudio:
On shutdown we have to tell the VT output thread to skip all DECPS/MIDI sequences. _getMidiAudio uses no mutex to allow thread-safe access of _midiAudio from multiple threads. But luckily we can either rely on how it's only called when the console lock is held, or we simply add another mutex there.

But both options are bad IMO: We had a long history of deadlock in Windows Terminal due to the main thread acquiring the console lock. And technically we don't need one, since all we want to do is call BeginSkip which is already inherently thread-safe. I simply dropped the lazy creation of MidiAudio which now allows me to blindly call BeginSkip witout if conditions and checks. I think this makes the code simpler and more robust. But it requires us to pass the HWND on all calls now for the internal lazy creation of DirectSound types.

Another downside is that I had to sprinkle the DirectSound headers into a bunch of .cpp files so that the destructor compiles without "undefined type" errors. I tried declaring a custom MidiAudio::~MidiAudio destructor, but that didn't make the compiler treat the destructor as an "extern" symbol either. I don't get why std::unique_ptr allows such type erasure where the compiler doesn't need to know the destructor definition, but Microsoft::Wrl and wil::com_ptr can't do that...

This is OK for now. We can reevalute the header situation in the future. :)

DHowett · 2022-10-13T21:41:03Z

@j4james FYI This might seem like a weird thing to do, but we got a report of the potential for DECPS blocking to be used as part of deceiving users into thinking that their session has hung (and they can't get out of it.)

j4james · 2022-10-13T23:58:11Z

OK I'll try and take a look at this over the weekend - it's going to take me a while to figure out how it works. It doesn't seem obvious to me at first glance how you can get away without locking the MidiAudio class.

But I do like the idea of being able to stop the sound with Ctrl+C.

lhecker · 2022-10-14T02:45:09Z

BTW the original issue mentions:

The terminal’s sound buffer can store 16 notes of specified volume and duration. Additional sound controls are held in the communications input buffer invoking receive data flow control as needed.

Unfortunately I don't think it was ever specifically mentioned how the VT520 behaved. Did it block IO until a control sequence finished playing or did it allow up to 16 notes to be queued up in a small circular buffer?
Because I feel like DECPS would be way more useful if it didn't block IO on every note. If you'd use it right now as - for instance - an indicator for a non-zero exit code, then it would always kinda freeze up your terminal briefly... 🤔

j4james · 2022-10-14T20:34:11Z

Unfortunately I don't think it was ever specifically mentioned how the VT520 behaved. Did it block IO until a control sequence finished playing or did it allow up to 16 notes to be queued up in a small circular buffer?

As far we could establish on the VT525 that jerch tested, that 16 note buffer was not a thing. The DECPS sequence could only accept a single note, and it blocked until the note had finished playing. The WT implementation extends that to allow for multiple notes to be specified in a single DECPS sequence (as allowed for in the documentation), but otherwise matches the VT525 blocking behavior, i.e. we block until they've all finished playing.

These details were uncovered in the comment thread here: jerch/xterm.js#1 (comment). It's really just the three messages at that point in the thread that are relevant to DECPS - we went on to discuss a bunch of unrelated audio stuff after that.

j4james · 2022-10-15T11:17:37Z

I've had a chance to play around with this PR, and try and figure out how it works without locking, i.e. what is keeping the StateMachine, Renderer, etc. open until it's safe to shutdown. So what I did was place some OutputDebugString calls in the constructors and destructors of various classes (TermControl, StateMachine, Renderer, etc.).

When I open a tab, I see the constructors being called, but when I close a tab, nothing. Keep opening and closing tabs - lots of construction, but no destruction. So I close the whole terminal and suddenly there's a long string of destructor calls to free up all those tabs that had previously been used.

So it seems the trick is that we're just not releasing any memory until the whole app is closed? That's surely not intentional is it? If anyone is using the terminal for an extended period of time it's just going to keep using more and more memory. Or am I misunderstanding something here?

And in the case of conhost, nothing ever gets destroyed as far I can tell. I suppose that is OK when the whole app is exiting anyway, but I'd like to know if that was an intentional optimization rather than a bug that we're just ignoring.

Anyway I'm assuming this change in behavior was introduced at some point in the past, so it's not technically a problem with this PR, but I'm not certain this PR would work without that change being in place.

lhecker · 2022-10-15T13:29:31Z

@j4james I opened #14228 to fix the memory leak in WT. Thanks for pointing it out! 😊

lhecker · 2022-10-15T13:36:05Z

And in the case of conhost, nothing ever gets destroyed as far I can tell. I suppose that is OK when the whole app is exiting anyway, but I'd like to know if that was an intentional optimization rather than a bug that we're just ignoring.

It was "intentional". We use ExitProcess() in ServiceLocator::RundownAndExit to exit immediately. I tried using exit() once to do a clean exit, but that didn't work at all. The primary problem is the heavy reliance on the Globals object and everything it ever touched to always exist. So for instance at one point I fixed a random crash that was caused by the CursorBlinker, because it kept blinking even when we were in the process of shutting down and the Renderer was already a nullptr. The issue is so bad that we had to add the #ifndef NDEBUG here:

terminal/src/interactivity/win32/window.cpp

Lines 68 to 81 in 1f19ed0

    
           Window::~Window() 
        
           { 
        
               // MSFT:40226902 - HOTFIX shutdown on OneCore, by leaking the renderer, thereby 
        
               // reducing the change for existing race conditions to turn into deadlocks. 
        
           #ifndef NDEBUG 
        
               delete pGdiEngine; 
        
           #if TIL_FEATURE_CONHOSTDXENGINE_ENABLED 
        
               delete pDxEngine; 
        
           #endif 
        
           #if TIL_FEATURE_CONHOSTATLASENGINE_ENABLED 
        
               delete pAtlasEngine; 
        
           #endif 
        
           #endif 
        
           }

j4james · 2022-10-15T19:17:20Z

OK, with PR #14228 applied, I've taken another look at this, and I think I understand how it works now. In case anyone else shared my confusion, the reason we don't need a lock is because the ConptyConnection::Close method waits for the output thread to exit, and that Close is now called from the ControlCore::Close (I think since PR #13882), so there should be no risk of anything being destroyed while the midi code is still finishing off.

With that understood, and the understanding that conhost is just going to exit immediately, I'm happy with this approach. I suspect there might be weird edge cases where we could get a bit of a delay in closing, but I think the simpler implementation more than makes up for that. Also I love the Ctrl+C functionality.

I have a few minor nits on the PR which I'll comment on later, but I don't think there's anything blocking.

j4james

Looks good to me. Just some comment updates, and clean up of obsolete code. I don't like the MidiDuration thing, but feel free to ignore that feedback if you disagree.

src/terminal/adapter/DispatchTypes.hpp

j4james · 2022-10-15T23:05:26Z

src/audio/midi/MidiAudio.cpp

    // By waiting on the shutdown future with the duration of the note, we'll
    // either be paused for the appropriate amount of time, or we'll break out
    // of the wait early if we've been shutdown.
-    _shutdownFuture.wait_for(duration);
+    _skip.wait(::base::saturated_cast<DWORD>(duration.count()));


This comment could do with an update. Maybe something like "By waiting on the skip event...", and possibly also mention that it applies to both shutdown and skipping notes.

j4james · 2022-10-15T23:07:10Z

src/cascadia/TerminalControl/ControlCore.cpp

-        // We create the audio instance on demand, and lock it for the duration
-        // of the note output so it can't be destroyed while in use.
-        auto& midiAudio = _getMidiAudio();
-        midiAudio.Lock();
-
        // We then unlock the terminal, so the UI doesn't hang while we're busy.
        auto& terminalLock = _terminal->GetReadWriteLock();
        terminalLock.unlock();


With the code above removed, this comment doesn't make sense. Maybe just need to drop the "then".

j4james · 2022-10-15T23:08:56Z

src/cascadia/TerminalControl/ControlCore.cpp

        // Once complete, we reacquire the terminal lock and unlock the audio.
        // If the terminal has shutdown in the meantime, the Unlock call
        // will throw an exception, forcing the thread to exit ASAP.
        terminalLock.lock();
-        midiAudio.Unlock();


Again this comment doesn't make sense with the Unlock code removed. Maybe just drop everything after "reacquire the terminal lock".

j4james · 2022-10-15T23:09:55Z

src/host/outputStream.cpp

-    // We create the audio instance on demand, and lock it for the duration
-    // of the note output so it can't be destroyed while in use.
-    auto& midiAudio = ServiceLocator::LocateGlobals().getConsoleInformation().GetMidiAudio();
-    midiAudio.Lock();
-
    // We then unlock the console, so the UI doesn't hang while we're busy.
    UnlockConsole();


Same deal as the comments in ControlCore::_terminalPlayMidiNote. Just drop the "then".

j4james · 2022-10-15T23:13:51Z

src/host/outputStream.cpp

    // Once complete, we reacquire the console lock and unlock the audio.
    // If the console has shutdown in the meantime, the Unlock call
    // will throw an exception, forcing the thread to exit ASAP.
    LockConsole();
-    midiAudio.Unlock();


Same as ControlCore::_terminalPlayMidiNote again. Just drop everything after "reacquire the console lock".

j4james · 2022-10-15T23:19:54Z

src/audio/midi/MidiAudio.cpp

    {
-        throw Microsoft::Console::VirtualTerminal::StateMachine::ShutdownException{};


Since we aren't using this exception anymore, we should get rid of the definition in statemachine.hpp, and the other bits of code associated with it that were added in PR #13208. There was a catch that was added to ControlCore::_connectionOutputHandler and another in StateMachine::_SafeExecute. You may also need to add back a bunch of noexcepts in the StateMachine class if the audit complains.

carlos-zamora · 2022-10-17T19:19:23Z

src/host/server.h

-    std::unique_ptr<MidiAudio> _midiAudio;
+    MidiAudio _midiAudio;


Curious: why? Don't we want this to be a unique_ptr to simplify destruction?

Actually I removed the unique_ptr to simplify destruction. The reasoning behind this is annoying and difficult and explained in detail in this comment: #14214 (comment)

But the gist of it is: In order to correctly shut down without race conditions we either need to protect access to _midiAudio with a mutex or remove the unique_ptr. The latter is the safer alternative.

src/host/outputStream.cpp

carlos-zamora · 2022-10-17T20:10:26Z

src/cascadia/TerminalControl/ControlCore.cpp

+        if (ch == L'\x3') // Ctrl+C or Ctrl+Break
+        {
+            _handleControlC();
+        }


Hmm... You want this behavior to be a part of the terminal handling it right? Like, if a user binds an action to ctrl+c, the action should be executed and this new behavior shouldn't happen right?

If that's the case, I suggest taking a look at ControlCore::TrySendKeyEvent(). By that point, we know that the input wasn't handled by an action so we can intercept it right before it goes to the shell (NOTE: this is where we have logic to update the selection from the keyboard when not in mark mode).

DHowett · 2022-10-17T21:06:20Z

Like, if a user binds an action to ctrl+c, the action should be executed and this new behavior shouldn't happen right?

Yeah, unfortunately this is correct. It should be overridable by an action just like actual ^C is.

lhecker · 2022-10-18T15:04:41Z

Yeah, unfortunately this is correct. It should be overridable by an action just like actual ^C is.

@DHowett @carlos-zamora Since a handled action returns true in the key event handler the character event handler is never called. So for instance if I run a DECPS sequence and select some text, pressing Ctrl+C won't cancel the sequence. I think the character event handler is a better fit than the key event one since C and Break have two different virtual keys and the behavior is a bit awkward (Ctrl+Break is 0x03 (VK_CANCEL), but Ctrl+C isn't, it's 0x43, aka 'C'). On the other hand in the character handler it's just a single ch == L'\x3'.

DHowett

I'm cool with this. Thank you!

DHowett · 2022-10-18T22:38:54Z

src/cascadia/TerminalControl/ControlCore.cpp

        auto& terminalLock = _terminal->GetReadWriteLock();
        terminalLock.unlock();

        // This call will block for the duration, unless shutdown early.
-        midiAudio.PlayNote(noteNumber, velocity, duration);
+        _midiAudio.PlayNote(reinterpret_cast<HWND>(_owningHwnd), noteNumber, velocity, std::chrono::duration_cast<std::chrono::milliseconds>(duration));


nit: _owningHwnd will change at runtime in the future; is MidiAudio prepared to handle that?

The device will be reinitialized if the HWND changes. But we don't expect this to happen too often right?

You're right, we don't!

DHowett · 2022-10-18T22:39:44Z

src/cascadia/TerminalControl/ControlCore.cpp

+            // Ensure Close() doesn't hang, waiting for MidiAudio to finish playing an hour long song.
+            _midiAudio.BeginSkip();


This is OK for now. We can reevalute the header situation in the future. :)

DHowett · 2022-10-18T22:41:04Z

@msftbot merge this in 15 minutes

ghost · 2022-10-18T22:41:10Z

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Tue, 18 Oct 2022 22:56:08 GMT, which is in 15 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett · 2022-10-31T22:18:03Z

I'm admin-merging with one Team sign-off since @j4james is the local area expert & I trust his review here 😄

Thanks!

lhecker · 2022-10-31T22:29:50Z

src/host/output.cpp

@@ -512,8 +512,6 @@ void CloseConsoleProcessState()

    HandleCtrlEvent(CTRL_CLOSE_EVENT);

-    gci.ShutdownMidiAudio();


FYI, reading another PR, I just realized that this change isn't quite correct. This should still call midiAudio.BeginSkip(), just like ControlCore does on shutdown.

Silent MIDI notes can be used to seemingly deny a user's input for long durations (multiple minutes). This commit improves the situation by ignoring all DECPS sequences for a second when Ctrl+C/Ctrl+Break is pressed. Additionally it fixes a regression introduced in 666c446: When we close a tab we need to unblock/shutdown `MidiAudio` early, so that `ConptyConnection::Close()` can run down as fast as possible. * In pwsh in Windows Terminal 1.16 run ``while ($True) { echo "`e[3;8;3,~" }`` * Ctrl+C doesn't do anything ❎ * Closing the tab doesn't do anything ❎ * With these modifications in Windows Terminal: * Ctrl+C stops the output ✅ * Closing the tab completes instantly ✅ * With these modifications in OpenConsole: * Ctrl+C stops the output ✅ * Closing the window completes instantly ✅ (cherry picked from commit b4fce27) Service-Card-Id: 86518584 Service-Version: 1.15

Silent MIDI notes can be used to seemingly deny a user's input for long durations (multiple minutes). This commit improves the situation by ignoring all DECPS sequences for a second when Ctrl+C/Ctrl+Break is pressed. Additionally it fixes a regression introduced in 666c446: When we close a tab we need to unblock/shutdown `MidiAudio` early, so that `ConptyConnection::Close()` can run down as fast as possible. * In pwsh in Windows Terminal 1.16 run ``while ($True) { echo "`e[3;8;3,~" }`` * Ctrl+C doesn't do anything ❎ * Closing the tab doesn't do anything ❎ * With these modifications in Windows Terminal: * Ctrl+C stops the output ✅ * Closing the tab completes instantly ✅ * With these modifications in OpenConsole: * Ctrl+C stops the output ✅ * Closing the window completes instantly ✅ (cherry picked from commit b4fce27) Service-Card-Id: 86518585 Service-Version: 1.16

ghost · 2022-12-14T20:42:27Z

🎉Windows Terminal v1.15.3465.0 and v1.15.3466.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2022-12-14T20:44:40Z

🎉Windows Terminal Preview v1.16.3463.0 and v1.16.3464.0 has been released which incorporates this pull request.:tada:

Handy links:

lhecker added Product-Conhost For issues in the Console codebase Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Priority-1 A description (P1) labels Oct 13, 2022

Skip DECPS/MIDI output on Ctrl+C/Break

fc3d134

lhecker force-pushed the dev/lhecker/fix-midi-freezes branch from 6841ae1 to fc3d134 Compare October 13, 2022 20:59

lhecker commented Oct 13, 2022

View reviewed changes

DHowett requested a review from j4james October 13, 2022 21:38

Simplify SetTimer, Fix x86 build

8eba770

j4james approved these changes Oct 15, 2022

View reviewed changes

carlos-zamora reviewed Oct 17, 2022

View reviewed changes

Address feedback

e5643c1

DHowett approved these changes Oct 18, 2022

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 18, 2022

DHowett merged commit b4fce27 into main Oct 31, 2022

DHowett deleted the dev/lhecker/fix-midi-freezes branch October 31, 2022 22:18

DHowett added this to To Cherry Pick in 1.15 Servicing Pipeline via automation Oct 31, 2022

DHowett added this to To Cherry Pick in 1.16 Servicing Pipeline via automation Oct 31, 2022

lhecker commented Oct 31, 2022

View reviewed changes

DHowett moved this from To Cherry Pick to Cherry Picked in 1.15 Servicing Pipeline Dec 1, 2022

DHowett moved this from To Cherry Pick to Cherry Picked in 1.16 Servicing Pipeline Dec 1, 2022

DHowett moved this from Cherry Picked to Shipped in 1.16 Servicing Pipeline Dec 1, 2022

		// Ensure Close() doesn't hang, waiting for MidiAudio to finish playing an hour long song.
		_midiAudio.BeginSkip();

		{
		throw Microsoft::Console::VirtualTerminal::StateMachine::ShutdownException{};

		@@ -512,8 +512,6 @@ void CloseConsoleProcessState()

		HandleCtrlEvent(CTRL_CLOSE_EVENT);

		gci.ShutdownMidiAudio();

Skip DECPS/MIDI output on Ctrl+C/Break #14214

Skip DECPS/MIDI output on Ctrl+C/Break #14214

Conversation

lhecker commented Oct 13, 2022 • edited

Validation Steps Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DispatcherQueueTimer.Start Method

Definition

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Oct 13, 2022

j4james commented Oct 13, 2022

lhecker commented Oct 14, 2022

j4james commented Oct 14, 2022

j4james commented Oct 15, 2022

lhecker commented Oct 15, 2022 • edited

lhecker commented Oct 15, 2022 • edited

j4james commented Oct 15, 2022

j4james left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Oct 17, 2022 • edited

lhecker commented Oct 18, 2022 • edited

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Oct 18, 2022

ghost commented Oct 18, 2022

DHowett commented Oct 31, 2022

lhecker Oct 31, 2022 • edited

Choose a reason for hiding this comment

ghost commented Dec 14, 2022

ghost commented Dec 14, 2022

lhecker commented Oct 13, 2022 •

edited

lhecker commented Oct 15, 2022 •

edited

lhecker commented Oct 15, 2022 •

edited

DHowett commented Oct 17, 2022 •

edited

lhecker commented Oct 18, 2022 •

edited

lhecker Oct 31, 2022 •

edited