Gui.cs: UI-Catalog Progress.cs

[BDisp]

If we quit of the Progress scenario when the timer is running sometimes the application crash.

[tig]

Yep, just rerpro'd!

[tig]

This is due to #523.

I need to take a closer look at that cleanup. I may not need to null Mainloop.

[tig]

FWIW, this is the real cause:

https://docs.microsoft.com/en-us/dotnet/api/system.timers.timer?view=netcore-3.1

Elapsed events can occur after the Dispose or Stop method has been called or after the Enabled property has been set to false, because the signal to raise the Elapsed event is always queued for execution on a thread pool thread. One way to resolve this race condition is to set a flag that tells the event handler for the Elapsed event to ignore subsequent events.

One could argue the bug is in Progress.cs and that it should be coded defensively for this:

            _timer = new Timer ((o) => {
                Application.MainLoop?.Invoke (() => Pulse ());
            }, null, 0, 20);

instead of this:

            _timer = new Timer ((o) => {
                Application.MainLoop.Invoke (() => Pulse ());
            }, null, 0, 20);

But I think we should do everything we can to make Terminal.Gui resilient to bugs like this. So I'm NOT going to fix this in Progress.cs (so we have a test case). But I will put a comment in. Ok?

[BDisp]

Perhaps in your implementation of the Disposewhen closing a toplevel you will have a variable that prevents Application.MainLoop.Invoke (() => Pulse ()) from being executed.

[tig]

Now that I've thought on it a bit more (coffee is catching up; I slept in this AM)...

My suggestion above is bad. If Shutdown has been called, the state of the mainloop is indeterminate. Calling Mainloop.Invoke in that case could do more bad things that could lead to even harder to find bugs. I'm remembering a tenet of good systems programming which is "Fail early and fail fast.".

So, I am going to fix this in Progress.c and I'm NOT going to change anything in the Shutdown code because it is actually correct.

[BDisp]

There is a bool variable running that is false it the Application is not running but isn't accessible in Progress, of course.

[tig]

There is a bool variable running that is false it the Application is not running but isn't accessible in Progress, of course.

Yep, that's right. However, the problem with using Running (as a public property) is that it actually is NOT a deterministic way of telling whether the Mainloop is running or not because there is no thread-safe protection for when it's read and changed. I believe just exposing it as a public would actually cause more problems.

For now, the best thing we can do is ensure all of this code cleans up properly. Longer term, I have some ideas about how to re-factor this to be more robust.