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

• CLA signed. If not, go over here 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_orders.

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