beto-rodriguez / LiveCharts2

Simple, flexible, interactive & powerful charts, maps and gauges for .Net, LiveCharts2 can now practically run everywhere Maui, Uno Platform, Blazor-wasm, WPF, WinForms, Xamarin, Avalonia, WinUI, UWP.
https://livecharts.dev
MIT License
4.23k stars 546 forks source link

Concurrency issue when clicking on chart with high frequency data updates #1081

Closed EricZeiberg closed 1 year ago

EricZeiberg commented 1 year ago

Describe the bug I have a chart which receives new data points at around 2KHz (2000/s) on a separate thread and plots them.

While performance is mostly fine, I noticed that when clicking on the chart, it will (after a couple clicks), produce a System.InvalidOperationException (Collection Was Modified) exception and crash. This is most likely due to one thread adding and removing data points from the ObservableCollection<ObservablePoint> array and the UI thread trying to read from the same collection.

I'm using a thread lock and the SyncContext property to prevent this but the click handler code doesn't seem to utilize this. I have tooltips turned off as well.

To Reproduce Steps to reproduce the behavior:

  1. Create a chart and add points to it at a high frequency on a separate thread (non-UI thread)
  2. Click a couple times anywhere on the chart window
  3. Observe the library crash

Expected behavior The chart should not crash.

Desktop:

Additional context After debugging, it seems like the problematic code is here, in Kernel/Extensions.cs.

public static ChartPoint? FindClosestTo(this IEnumerable<ChartPoint> points, LvcPoint point)
    {
        var fp = new LvcPoint((float)point.X, (float)point.Y);

        return points
            .Select(p => new
            {
                distance = p.DistanceTo(fp),
                point = p
            })
            .OrderBy(p => p.distance)
            .FirstOrDefault()?.point;
    }

This LINQ expression does not lock the SyncContext object before it performs a Select() query.

beto-rodriguez commented 1 year ago

I am not able to reproduce this, it probably means that the concurrency issue could be on your side, please could you provide a repository with the issue?

You can test the library by cloning this repo and go to the multithreading sample, then to create a similar case to yours just modify the ReadData method to:

private async void ReadData()
    {
        await Task.Delay(1000);

        while (true)
        {
            await Task.Delay(_delay);

            for (var i = 0; i < 2000; i++)
            {
                _current = Interlocked.Add(ref _current, _r.Next(-9, 10));
                lock (Sync)
                {
                    _values.Add(_current);
                    _values.RemoveAt(0);
                }
                await Task.Delay(1);
            }
        }
    }

Finally set these 2 variables in the constructor of the VM:

_delay = 1000;
var readTasks = 5;

With those settings we are adding/removing about 10,000 points per seconds I event can use tooltips without issues:

multithreading

I will close this for now since there is no evidence that the library has an issue here, please feel free to reply or open a new issue with a repository where I can see the issue and help you better.

EricZeiberg commented 1 year ago

I cloned the latest version of the repo and was able to reproduce the issue in the Multithreading example. Please try clicking on the graph a bunch of times, it should crash with an InvalidOperationException on line 370 of the DataFactory.cs class.

beto-rodriguez commented 1 year ago

Which platform is it?

EricZeiberg commented 1 year ago

Avalonia 11. Was also breaking with the 0.10.x version. Sorry, forgot to mention this.

beto-rodriguez commented 1 year ago

Ok, I can reproduce on Avalonia, thanks for the info. I will investigate, I have noticed that the Avalonia team improved a lot of things in v11, the sample I mentioned above was not even working in Avalonia 0.10, notice this issue does not happen on the rest of the platforms in this repo.

I need to find if the issue is on Live Charts or in the Avalonia side.

as a workaround just invoke the changes in the UI thread, I am sure that has no issues on Avalonia.

EricZeiberg commented 1 year ago

Thanks for investigating.

Yes, I can invoke in UI thread but posting on that thread 2000 times / sec causes performance issues on my end.

beto-rodriguez commented 1 year ago

You were right, when you click the chart, it tries to invoke the chart events, but since the data is changing on another thread this exception is thrown, it is strange that it does not throw in the rest on the platforms 😥

The referenced commit fixes this, this is just for now, I think we can do much better and completely ignore the click event when there are no handlers subscribed to the chart events.

EricZeiberg commented 1 year ago

Thanks for the quick fix! When should I expect a build with this change to be published to NuGet?

beto-rodriguez commented 1 year ago

It is included now in 2.0.0-beta.850.1