Files
terminal/src/renderer/base/thread.cpp
greg904 2f60cf0e91 Fix RenderThread's notify mechanism (#4698)
## Summary of the Pull Request

Fix a bug where the `Renderer::PaintFrame` method:
1. is not called until the next `RenderThread::NotifyThread` call but needs to be called because there the terminal was updated (theoretical bug)
2. is called twice but needs to be called only once (verified bug)

## References

The bug was introduced by #3511.

## PR Checklist
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA

## Detailed Description of the Pull Request / Additional comments

### Before

#### First bug

In the original code, `_fNextFrameRequested` is set to `true` in render thread because `std::atomic_flag::test_and_set` is called.

This is wrong because it means that the render thread will render the terminal again even if there is no change after the last render.

I think the the goal was to load the boolean value for `_fNextFrameRequested` to check whether the thread should sleep or not.

The problem is that there is no method on `std::atomic_flag` to load its boolean value. I guess what happened was that the "solution" that was found was to use `std::atomic_flag::test_and_set`, followed by `std::atomic_flag::clear` if the value was `false` originally (if `std::atomic_flag::test_and_set` returned `false`) to restore the original value. I guess that this was believed to be equivalent to just a simple load, without doing any change to the value because it restores it at the end.

But it's not: this is dangerous because if the value is changed to `true` between the call to `std::atomic_flag::test_and_set` and the call to `std::atomic_flag::clear`, then the value ends up being `false` at the end which is wrong because we don't want to change it! And if that value ends up being `false`, it means that we miss a render because we will wait on `_hEvent` during the next iteration on the render thread.

Well actually, here, this not even a problem because when that code is ran, `_fPainting` is `false` which means that the other thread that modifies the `_fNextFrameRequested` value through `RenderThread::NotifyPaint` will not actually modify `_fNextFrameRequested` but rather call `SetEvent` (see the method's body).

But wait! There is a problem there too! `std::atomic_flag::test_and_set` is called for `_fPainting` which sets its value to `true`. It was probably unintended. So actually, the next call to `RenderThread::NotifyPaint` _will_ end up modifying `_fNextFrameRequested` which means that the data race I was talking about _might_ happen!

#### Second bug

Let's go back a little bit in my explanation. I was talking about the fact that:

> I guess what happened was that the "solution" that was found was to use `std::atomic_flag::test_and_set`, followed by `std::atomic_flag::clear` if the value was `false` originally (if `std::atomic_flag::test_and_set` returned `false`) to restore the original value.

The problem is that the reverse was done in the implementation: `std::atomic_flag::clear` is called if the value was _`true`_ originally!

So at this point, if the value of `_fNextFrameRequested` was `false`, then `std::atomic_flag::test_and_set` sets its is set to `true` and returns `false`. So for the next iteration, `_fNextFrameRequested` is `true` and the render thread will re-render but that was not needed.

### After

I used `std::atomic<bool>` instead of `std::atomic_flag` for `_fNextFrameRequested` and the other atomic field because it has a `load` and a `store` method so we can actually load the value without changing it.

I also replaced `_fPainting` by `_fWaiting`, which is basically the opposite of `_fPainting` but stays `true` for a little shorter than `_fPainting` would stay `false`. Indeed, I think that it makes more sense to directly wrap/scope _just_ the call to `WaitForSingleObject` by setting my atomic variable to `true` _just_ before and to `false` _just_ after because:
* It makes more sense while you're reading the code: it's easier IMO to understand what the purpose of `_fWaiting` is (that is, to call `SetEvent` from `RenderThread::NotifyPaint` if it's `true`).
* It's probably a tiny bit better for performance because it will become `true` for a little shorter which means less calls to `SetEvent`.

#### Warning

I don't really understand [std::memory_order](https://en.cppreference.com/w/cpp/atomic/memory_order)s.

So I used the default one (`std::memory_order_seq_cst`) which is the safest.

I believe that if no read or write are reordered in the two threads (`RenderThread::NotifyPaint` and `RenderThread::_ThreadProc`), then the code I wrote will behave correctly.

I think that `std::memory_order_seq_cst` enforces that so it should be fine, but I'm not sure.

## Validation Steps Performed

**I tried to reproduce the second bug that I described in the first section of this PR.**

I put a breakpoint on `RenderThread::NotifyPaint` and on `Renderer::PaintFrame`. Initially they are disabled. Then I ran the terminal in Release mode, waited a bit for the prompt to display and the cursor to start blinking. Then I enabled the breakpoints.

### Before

Each `RenderThread::NotifyPaint` is followed by 2 `Renderer::PaintFrame` calls.  

### After

Each `RenderThread::NotifyPaint` is followed by 1 `Renderer::PaintFrame` call. ✔️
2020-02-27 18:29:21 +00:00

290 lines
9.4 KiB
C++
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
#include "precomp.h"
#include "thread.hpp"
#pragma hdrstop
using namespace Microsoft::Console::Render;
RenderThread::RenderThread() :
_pRenderer(nullptr),
_hThread(nullptr),
_hEvent(nullptr),
_hPaintCompletedEvent(nullptr),
_fKeepRunning(true),
_hPaintEnabledEvent(nullptr),
_fNextFrameRequested(false),
_fWaiting(false)
{
}
RenderThread::~RenderThread()
{
if (_hThread)
{
_fKeepRunning = false; // stop loop after final run
SignalObjectAndWait(_hEvent, _hThread, INFINITE, FALSE); // signal final paint and wait for thread to finish.
CloseHandle(_hThread);
_hThread = nullptr;
}
if (_hEvent)
{
CloseHandle(_hEvent);
_hEvent = nullptr;
}
if (_hPaintEnabledEvent)
{
CloseHandle(_hPaintEnabledEvent);
_hPaintEnabledEvent = nullptr;
}
if (_hPaintCompletedEvent)
{
CloseHandle(_hPaintCompletedEvent);
_hPaintCompletedEvent = nullptr;
}
}
// Method Description:
// - Create all of the Events we'll need, and the actual thread we'll be doing
// work on.
// Arguments:
// - pRendererParent: the IRenderer that owns this thread, and which we should
// trigger frames for.
// Return Value:
// - S_OK if we succeeded, else an HRESULT corresponding to a failure to create
// an Event or Thread.
[[nodiscard]] HRESULT RenderThread::Initialize(IRenderer* const pRendererParent) noexcept
{
_pRenderer = pRendererParent;
HRESULT hr = S_OK;
// Create event before thread as thread will start immediately.
if (SUCCEEDED(hr))
{
HANDLE hEvent = CreateEventW(nullptr, // non-inheritable security attributes
FALSE, // auto reset event
FALSE, // initially unsignaled
nullptr // no name
);
if (hEvent == nullptr)
{
hr = HRESULT_FROM_WIN32(GetLastError());
}
else
{
_hEvent = hEvent;
}
}
if (SUCCEEDED(hr))
{
HANDLE hPaintEnabledEvent = CreateEventW(nullptr,
TRUE, // manual reset event
FALSE, // initially signaled
nullptr);
if (hPaintEnabledEvent == nullptr)
{
hr = HRESULT_FROM_WIN32(GetLastError());
}
else
{
_hPaintEnabledEvent = hPaintEnabledEvent;
}
}
if (SUCCEEDED(hr))
{
HANDLE hPaintCompletedEvent = CreateEventW(nullptr,
TRUE, // manual reset event
TRUE, // initially signaled
nullptr);
if (hPaintCompletedEvent == nullptr)
{
hr = HRESULT_FROM_WIN32(GetLastError());
}
else
{
_hPaintCompletedEvent = hPaintCompletedEvent;
}
}
if (SUCCEEDED(hr))
{
HANDLE hThread = CreateThread(nullptr, // non-inheritable security attributes
0, // use default stack size
s_ThreadProc,
this,
0, // create immediately
nullptr // we don't need the thread ID
);
if (hThread == nullptr)
{
hr = HRESULT_FROM_WIN32(GetLastError());
}
else
{
_hThread = hThread;
}
}
return hr;
}
DWORD WINAPI RenderThread::s_ThreadProc(_In_ LPVOID lpParameter)
{
RenderThread* const pContext = static_cast<RenderThread*>(lpParameter);
if (pContext != nullptr)
{
return pContext->_ThreadProc();
}
else
{
return (DWORD)E_INVALIDARG;
}
}
DWORD WINAPI RenderThread::_ThreadProc()
{
while (_fKeepRunning)
{
WaitForSingleObject(_hPaintEnabledEvent, INFINITE);
if (!_fNextFrameRequested.exchange(false))
{
// <--
// If `NotifyPaint` is called at this point, then it will not
// set the event because `_fWaiting` is not `true` yet so we have
// to check again below.
_fWaiting.store(true);
// check again now (see comment above)
if (!_fNextFrameRequested.exchange(false))
{
// Wait until a next frame is requested.
WaitForSingleObject(_hEvent, INFINITE);
}
// <--
// If `NotifyPaint` is called at this point, then it _will_ set
// the event because `_fWaiting` is `true`, but we're not waiting
// anymore!
// This can probably happen quite often: imagine a scenario where
// we are waiting, and the terminal calls `NotifyPaint` twice
// very quickly.
// In that case, both calls might end up calling `SetEvent`. The
// first one will resume this thread and the second one will
// `SetEvent` the event. So the next time we wait, the event will
// already be set and we won't actually wait.
// Because it can happen often, and because rendering is an
// expensive operation, we should reset the event to not render
// again if nothing changed.
_fWaiting.store(false);
// see comment above
ResetEvent(_hEvent);
}
ResetEvent(_hPaintCompletedEvent);
LOG_IF_FAILED(_pRenderer->PaintFrame());
SetEvent(_hPaintCompletedEvent);
// extra check before we sleep since it's a "long" activity, relatively speaking.
if (_fKeepRunning)
{
Sleep(s_FrameLimitMilliseconds);
}
}
return S_OK;
}
void RenderThread::NotifyPaint()
{
if (_fWaiting.load())
{
SetEvent(_hEvent);
}
else
{
_fNextFrameRequested.store(true);
}
}
void RenderThread::EnablePainting()
{
SetEvent(_hPaintEnabledEvent);
}
void RenderThread::WaitForPaintCompletionAndDisable(const DWORD dwTimeoutMs)
{
// When rendering takes place via DirectX, and a console application
// currently owns the screen, and a new console application is launched (or
// the user switches to another console application), the new application
// cannot take over the screen until the active one relinquishes it. This
// blocking mechanism goes as follows:
//
// 1. The console input thread of the new console application connects to
// ConIoSrv;
// 2. While servicing the new connection request, ConIoSrv sends an event to
// the active application letting it know that it has lost focus;
// 3.1 ConIoSrv waits for a reply from the client application;
// 3.2 Meanwhile, the active application receives the focus event and calls
// this method, waiting for the current paint operation to
// finish.
//
// This means that the new application is waiting on the connection request
// reply from ConIoSrv, ConIoSrv is waiting on the active application to
// acknowledge the lost focus event to reply to the new application, and the
// console input thread in the active application is waiting on the renderer
// thread to finish its current paint operation.
//
// Question: what should happen if the wait on the paint operation times
// out?
//
// There are three options:
//
// 1. On timeout, the active console application could reply with an error
// message and terminate itself, effectively relinquishing control of the
// display;
//
// 2. ConIoSrv itself could time out on waiting for a reply, and forcibly
// terminate the active console application;
//
// 3. Let the wait time out and let the user deal with it. Because the wait
// occurs on a single iteration of the renderer thread, it seemed to me that
// the likelihood of failure is extremely small, especially since the client
// console application that the active conhost instance is servicing has no
// say over what happens in the renderer thread, only by proxy. Thus, the
// chance of failure (timeout) is minimal and since the OneCoreUAP console
// is not a massively used piece of software, it didnt seem that it would
// be a good use of time to build the requisite infrastructure to deal with
// a timeout here, at least not for now. In case of a timeout DirectX will
// catch the mistake of a new application attempting to acquire the display
// while another one still owns it and will flag it as a DWM bug. Right now,
// the active application will wait one second for the paint operation to
// finish.
//
// TODO: MSFT: 11833883 - Determine action when wait on paint operation via
// DirectX on OneCoreUAP times out while switching console
// applications.
ResetEvent(_hPaintEnabledEvent);
WaitForSingleObject(_hPaintCompletedEvent, dwTimeoutMs);
}