Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Oct 31, 2022
Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Oct 13, 2022

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 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
@lhecker lhecker force-pushed the dev/lhecker/fix-midi-freezes branch from 6841ae1 to fc3d134 Compare October 13, 2022 20:59
Comment on lines +48 to +51
if (_skip.is_signaled())
{
return;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new trick.

Comment on lines +53 to +55
if (_hwnd != windowHandle)
{
throw Microsoft::Console::VirtualTerminal::StateMachine::ShutdownException{};
_initialize(windowHandle);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +406 to +409
if (ch == L'\x3') // Ctrl+C or Ctrl+Break
{
_handleControlC();
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

{
if (!_midiAudioSkipTimer)
{
_midiAudioSkipTimer = _dispatcher.CreateTimer();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +1517 to +1518
// Ensure Close() doesn't hang, waiting for MidiAudio to finish playing an hour long song.
_midiAudio.BeginSkip();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DHowett DHowett requested a review from j4james October 13, 2022 21:38
@DHowett
Copy link
Member

DHowett commented Oct 13, 2022

@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
Copy link
Collaborator

j4james commented Oct 13, 2022

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
Copy link
Member Author

lhecker commented Oct 14, 2022

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
Copy link
Collaborator

j4james commented Oct 14, 2022

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
Copy link
Collaborator

j4james commented Oct 15, 2022

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
Copy link
Member Author

lhecker commented Oct 15, 2022

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

@lhecker
Copy link
Member Author

lhecker commented Oct 15, 2022

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:

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
Copy link
Collaborator

j4james commented Oct 15, 2022

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.

Copy link
Collaborator

@j4james j4james left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Comment on lines 80 to 83
// 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()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 1396 to 1424
// 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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 1429 to 1412
// 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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 376 to 377
// 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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 384 to 391
// 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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
throw Microsoft::Console::VirtualTerminal::StateMachine::ShutdownException{};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines -160 to +159
std::unique_ptr<MidiAudio> _midiAudio;
MidiAudio _midiAudio;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Comment on lines +406 to +409
if (ch == L'\x3') // Ctrl+C or Ctrl+Break
{
_handleControlC();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

DHowett commented Oct 17, 2022

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
Copy link
Member Author

lhecker commented Oct 18, 2022

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'.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm cool with this. Thank you!

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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, we don't!

Comment on lines +1517 to +1518
// Ensure Close() doesn't hang, waiting for MidiAudio to finish playing an hour long song.
_midiAudio.BeginSkip();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DHowett
Copy link
Member

DHowett commented Oct 18, 2022

@msftbot merge this in 15 minutes

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

ghost commented Oct 18, 2022

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
Copy link
Member

DHowett commented Oct 31, 2022

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

Thanks!

@DHowett DHowett merged commit b4fce27 into main Oct 31, 2022
@DHowett DHowett deleted the dev/lhecker/fix-midi-freezes branch October 31, 2022 22:18
@@ -512,8 +512,6 @@ void CloseConsoleProcessState()

HandleCtrlEvent(CTRL_CLOSE_EVENT);

gci.ShutdownMidiAudio();
Copy link
Member Author

@lhecker lhecker Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

DHowett pushed a commit that referenced this pull request Dec 1, 2022
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
DHowett pushed a commit that referenced this pull request Dec 1, 2022
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
Copy link

ghost commented Dec 14, 2022

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

Handy links:

@ghost
Copy link

ghost commented Dec 14, 2022

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conhost For issues in the Console codebase Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants