Application stops responding

[kacperpikacz]

Describe the bug In less than minute the application is not responding anymore. No exception is thrown.

To Reproduce

using System.Collections.ObjectModel;
using System.Diagnostics;
using System.Text.RegularExpressions;
using Terminal.Gui;

namespace Playground
{
    public class Program
    {
        // Same issue with class List
        private static ObservableCollection<string> List = new();
        private static ListView ListView = new();
        private static Timer? _timer;

        public static void Main(string[] args)
        {
            List.CollectionChanged += (obj, args) =>
            {
                Application.MainLoop.Invoke(() =>
                {
                    if (List.Count == 100) List.RemoveAt(0);
                    ListView.MoveDown();
                    ListView.SetNeedsDisplay();
                });
            };

            ListView = new ListView()
            {
                Width = 16,
                Height = 12,

            };
            ListView.SetSource(List);

            ListView.Enter += (args) =>
            {
                // We can use Application.MainLoop.AddTimeout(timespan, callback) but it will behave the same.
                _timer = new Timer(Refresh, null, TimeSpan.Zero, TimeSpan.FromMilliseconds(2));
            };

            Application.Init();
            Application.Top.Add(ListView);
            Application.Run();
        }

        private static void Refresh(object? state)
        {
            Application.MainLoop.Invoke(() =>
            {
                List.Add(new Random().Next(100000, 999999).ToString());
            });

        }

    }
}

Desktop: Tested with Apple Silicon & Linux x64 and arm64

Additional context Tested with Windows ARM and the application is working correctly in 5min timeframe.

[tznind]

Can confirm working correctly on Windows

As I understand it you are adding a new object to list every 2 ms and refreshing the UI at that time.

You can try decoupling list modification from screen refresh e.g.

Can you try running this adjusted code and see if you have same issue, it limits screen refresh to 4 times a second (250ms) while the list modification speed remains the same:

using System.Collections.ObjectModel;
using Terminal.Gui;

namespace Playground
{
    public class Program
    {
        // Same issue with class List
        private static ObservableCollection<string> List = new();
        private static ListView ListView = new();
        private static Timer? _timer = null;

        public static void Main(string[] args)
        {

            // Move init to top before controls are created
            Application.Init();

            bool listChanged = false;

            // Update the list in 'real time'
            List.CollectionChanged += (obj, args) =>
            {
                listChanged = true;
                if (List.Count == 100)
                {
                      /* I've left this in, but do notice that you are already in a list changed callback and are 
                          making another modification - presumably resulting in re-entry.  This is likely to
                          compound performance issue i.e. we are now modfiying list every 1ms because
                          of re-entry.
                     */
                    List.RemoveAt(0);
                }
            };

            // Every 250 ms update the UI
            Application.MainLoop.AddTimeout(TimeSpan.FromMilliseconds(250), (m) =>
            {

                Application.MainLoop?.Invoke(() =>
                {
                    ListView.MoveEnd();
                    ListView.SetNeedsDisplay();
                    listChanged = false;
                });
                return true;
            });

            ListView = new ListView()
            {
                Width = 16,
                Height = 12,

            };
            ListView.SetSource(List);

            ListView.Enter += (args) =>
            {
                // Only ever start 1 timer
                if (_timer == null)
                {
                    // We can use Application.MainLoop.AddTimeout(timespan, callback) but it will behave the same.
                    _timer = new Timer(Refresh, null, TimeSpan.Zero, TimeSpan.FromMilliseconds(2));
                }
            };
            Application.Top.Add(ListView);
            Application.Run();
        }

        private static void Refresh(object? state)
        {
            Application.MainLoop.Invoke(() =>
            {
                List.Add(new Random().Next(100000, 999999).ToString());
            });

        }
    }
}

[kacperpikacz]

Tried with limiting screen refresh every 25 ms. Same behaviour using Application.MainLoop.AddTimeout() instead of Threading.Timer

using System;
using System.Collections.ObjectModel;
using System.Diagnostics;
using System.Text.RegularExpressions;
using Terminal.Gui;

namespace Playground
{
    public class Program
    {
        private static ObservableCollection<string> List = new();
        private static ListView ListView = new();
        private static List<Timer> _timers = new();

        public static void Main(string[] args)
        {
            Application.Init();

            ListView = new ListView()
            {
                Width = 16,
                Height = 12,

            };

            ListView.SetSource(List);
            ListView.Enter += (args) =>
            {
                Application.MainLoop.Invoke(() =>
                {
                    _timers.Add(new Timer(Generate, null, TimeSpan.Zero, TimeSpan.FromMilliseconds(1)));
                });

            };

            _timers.Add(new Timer(Refresh, null, TimeSpan.Zero, TimeSpan.FromMilliseconds(50)));

            Application.Top.Add(ListView);
            Application.Run();
        }

        private static void Generate(object? state)
        {
            Application.MainLoop.Invoke(() =>
            {
                List.Add(new Random().Next(100000, 999999).ToString());
            });
        }

        private static void Refresh(object? state)
        {
            Application.MainLoop.Invoke(() =>
            {
                ListView.MoveDown();
                ListView.SetNeedsDisplay();

                if (List.Count is 100) List.RemoveAt(0);
            });
        }

    }
}

After around 90 sec

https://github.com/user-attachments/assets/193315e1-c41c-4130-9558-6fd112f633cb

[tznind]

Try removing invokes on the list change events

 private static void Generate(object? state)
        {
            Application.MainLoop.Invoke(() =>
            {
                List.Add(new Random().Next(100000, 999999).ToString());
            });
        }

This basically says "every 1ms switch to UI thread and modify list". There is no reason to switch to the UI thread to do this. See in my above code the only invoke is when updating the view.

Likewise you dont need main loop invoke when setting up the timer for Generate.

Try my exact code or make the above changes and lets see. I think the performance is down to switching constantly to the UI loop every millisecond. Evidence for this is that when it slows down its not just the UI refresh that is slowing down, the generation of numbers is much slower too.

If worried about concurrent access to the list you can switch to one of the concurrent collections.

[dodexahedron]

To add to @tznind's analysis, which is spot on except for one little detail, the way that ListView works unconditionally is going to cause additional copies of your entire collection to be made, as a list. That little detail I mentioned is that the use of a concurrent collection by the consumer won't solve concurrency problems TG itself has, since the collection gets copied anyway, and has additional concurrency issues not directly related to the collections themselves, as well.

But the copy behavior means that, depending on size, there could be multiple heap allocations each time, not counting the ones that are happening for growth/modification of your source collections.

With that in mind, do try to limit how "real-time" you try to be with ListView, especially as the number of elements grows. And remember that 16ms is already 60FPS, so consider the utility of updating on really small timescales in the first place, anyway.

I'm pretty sure I've got an open issue for taking a look at how to improve the memory pressure situation under the hood for ListView, already. If not, it is on my personal list of things I want to address at some point, since it is a lot heavier than it needs to be.

If you're adding and removing a bunch of elements in a ListView, it is best, with the current implementation, to do all the modifications you need to your collection first, or at least do them in batches, and then provide the collection to the ListView.

Rapid modification will encounter the memory thrashing seen above, which compound with garbage generated by string handling in certain places, and is also likely to encounter concurrency issues that can result in deadlocks and race conditions, as we haven't yet addressed thread-safety for ListView and many other components, including some core parts of the infrastructure.

[dodexahedron]

As for Timer vs the TG Timeout struct... Yeah... I'm probably ripping that out while I'm doing the ConsoleDriver stuff I'm working on, so keep using built-in timer constructs if those are working better for you. The Timeout implementation has a lot of concurrency problems.

It seems to really only exist to support SpinnerView, anyway, and can be pretty easily replaced by a native timer construct, so the Timeout struct is almost 100% on the chopping block.

[tznind]

@dodexahedron just heads up that OP is using v1 library dependency (based on the code).

I saw you put tag on ticket for v2 - or was that just to address this kind of use case in v2?

[kacperpikacz]

I was doing some tests with v1_release/v1.5.0 and seems it behaves much better since I didn't get any ui lock/exception in a 2 hour timeframe. Even with modifying a ListView source every 5ms

@dodexahedron even if I apply lock while modifying the source of ListView I will get an Exception after few minutes in latest version of v1

[dodexahedron]

@dodexahedron just heads up that OP is using v1 library dependency (based on the code).

I saw you put tag on ticket for v2 - or was that just to address this kind of use case in v2?

Oops, I just mis-clicked, actually. Wrong tag.

I didn't really differentiate in my commentary since the issues in that area are mostly the same, though I probably should have specified my work related to it is v2 only.

My goal is to make things that can be made thread-safe (without horrendous effort or without introducing too many problems like high deadlock potential or way too much contention) thread-safe, and at least add remarks for types known not to be thread-safe that aren't fixed or won't be fixed.

[dodexahedron]

@dodexahedron even if I apply lock while modifying the source of ListView I will get an Exception after few minutes in latest version of v1

Yep. That's what I was saying about how you can't fix the concurrency problems in your code. They're in the library itself, and are caused by unsynchronized access to various things, for the most part.

Those are going to be addressed in 2.0, but that hasn't begun yet.

[BDisp]

This is a changed sample which is working as expected with the PR #3739.

#define TIMER

using System;
using System.Collections.ObjectModel;
using System.Threading;
using Terminal.Gui;

namespace Playground {
    public class public class Program {
        // No issue with class List as well
        private static ObservableCollection<string> List = new ();
        // Don't initialize here because it's initialized again bellow to avoid not disposed issues
        private static ListView ListView;
#if TIMER
        private static Timer? _timer;
#endif
        public static void Main(string[] args)
        {
            List.CollectionChanged += (obj, args) => {
                Application.MainLoop.Invoke (() => {
                    ListView.MoveEnd ();
                    ListView.SetNeedsDisplay ();
                });
            };

            ListView = new ListView () {
                Width = 16,
                Height = 12,

            };
            ListView.SetSource (List);

            ListView.Enter += (args) => {
#if TIMER
                if (_timer == null) {
                    _timer = new Timer (Refresh, null, TimeSpan.Zero, TimeSpan.FromMilliseconds (2));
                }
#else
                // We can use Application.MainLoop.AddTimeout(timespan, callback) as well.
                Application.MainLoop.AddTimeout (TimeSpan.FromMilliseconds (2), _ => {
                    Refresh (null);
                    return true;
                });
#endif
            };

            Application.Init ();
            Application.Top.Add (ListView);
            Application.Run ();
#if TIMER
            _timer?.Dispose ();
#endif
            // I recommend always using this
            Application.Shutdown ();
        }

        private void Refresh (object? state)
        {
            // No need to use Application.MainLoop.Invoke here because the List.CollectionChanged 
            // will be raised during this method

            // Move this to here to ensure there always have 100 items
            if (List.Count == 100) List.RemoveAt (0);

            List.Add (new Random ().Next (100000, 999999).ToString ());
        }
    }
}

[BDisp]

In v2, to avoid the above error it's need surround with a lock block. Note that this error was in the v2 because the ListView already uses a ObservableCollection as source and thus exposed to concurrency problems. Just to remember that the ListView.CollectionChanged event can be used instead of List.CollectionChanged in the v2.

    private void Refresh (object? state)
    {
        lock (_list)
        {
            if (_list.Count == 100)
            {
                _list.RemoveAt (0);
            }

            _list.Add (new Random ().Next (100000, 999999).ToString ());
        }
    }

[kacperpikacz]

@BDisp your recent PR fixes the issue. I will do some longer tests but looks like no more problems with my use case.