RoboSharp
RoboSharp copied to clipboard
tjscience
ObservableList<T> and WPF - Not Truly Safely Observable
I'm writing this into an issue instead of just fixing it since I don't want to include unnecessary components into the library unless its truly warranted (funny right? considering all my other changes).
The Problem:
Basically, the ObservableList<T> object was written to have a thread-safe list to enable the functionality of RoboQueue and ProgressEstimator. Without it, those two classes won't work at all.
The problem is for consumers that attempt to use it alongside WPF views. For infrequent updates, this object works fantastic. What I mean by 'infrequent updates' would be something like generating a popup, having the user select from the list, then those items get added to another list. Since all of those events are generated from the UI thread, and they also take time to resolve the opening/closing of the windows, the view is updated just fine.
The problem occurs when attempting to modify the list faster than the UI updates the list. You may get an exception such as:
System.InvalidOperationException: 'A collection Add event refers to item that does not belong to collection.'
When multiple events are dispatched to the UI, the UI handles them whenever it handles them. If the modification occurs while the UI is updating, you can get a fault due to the Count property (as read by the UI) being 6 when the UI starts displaying the items, but by the time the UI finishes iterating over the list there are 7 items.
An example of this would be if you had an ObservableList<string> that was holding the log lines in a custom implementation. As the custom implementation processes the files/folders, it will update this list, but it will likely update much quicker than the UI handles the CollectionChanged events. ( This is actually the exact scenario I ran into this issue - I was reacting to the ProcessedFile event in the ViewModel, and adding those to a list to update the View)
The Solution:
I've done some googling, and what I found was this article, and the corresponding library:
https://www.meziantou.net/thread-safe-observable-collection-in-dotnet.htm
https://github.com/meziantou/Meziantou.Framework/blob/main/src/Meziantou.Framework.WPF/Collections/
Implementing a design like above will require the <UseWPF>true</UseWPF> declaration in the project.
I did not want to add that requirement to this library without consensus of consumers and other maintainers.
Adding this will effectively add WPF requirement to any consumers that are not using the NetStandard version of this library, even if their project isn't using WPF (ex: they are doing a winforms or console project).
Potential Resolutions:
This library only uses this object privately, and is expsoing it via the ReadOnlyCollection object.
We could make it internal-only, therefore removing the issue from consumers by simply not allowing them to instantiate the object. This of course will break any implementations by consumers that rely on this object since it has been introduced. But it would force them to use an actual WPF-Safe object.
We could rename it to ConcurrentList<T>, then add a new project to the solution that provides the ObservableList<T> object for consumers in the RoboSharp namespace, using a similar implementation to the link above. This will only break consumers if they choose not to include that project reference. This also adds complexity to not only this library, but to consumers. As such, I don't think this is a great route when a library like the linked one above exists.
As of writing this, I say we leave it exposed, but rename it ConcurrentList to more accurately describe its use case. ObservableList<T> will be marked as Obsolete, and throw an exception if you try to instantiate it, describing why it threw. This will effectively alert consumers (by breaking their uses of it) that it is NOT UI-Safe. This change will allow it to still be utilized by custom implementations, but will require those projects be updated to the new object name.
(funny right? considering all my other changes).
I must admit that did make me smile - in a good way of course, all the work you have done on this project is very much appreciated!!
As for the question at hand, I am not really the best one to comment as I don't use or work with WPF much, my apps which use RoboSharp are all WinForms based so as long as whatever changes are made don't completely break (or at least have a fix) to work with WinForms then I bow down to your knowledge on this subject, I certainly understand the problem you explain and the proposed resolutions, but probably others more knowledgeable on WPF are better positioned to respond than I on this one. But certainly happy to test any propose resolutions to see how it may affect any of my WinForm projects (for info I use the net45 version of the DLL currently, but no reason I couldn't potentially move my app over to using netstandard2.1 or net5.0)
I'm actually currently testing a variation of the ImmutableList<T> object that I had linked. Thus far, its passing all its tests as far as what would be expected of a list. This will add a new class that is internal to this library, but will complete the thread-safety-goodness that ObservableList<T> was supposed to have originally.
Essentially, enumerating through the ConcurrentList<T> object will be actually enumerating through the ImmutableList<T> object behind the scenes, which hopefully resolves this issue.
These changes are now included in my PR btw
I would like to say that (I Believe) the problem this post brought up is now resolved in my PR.
I have bound a ListView control to the ConcurrentList.ReadOnlyList property, and the view is updated when the list is updated without any faults occurring. This may be doing it in a somewhat round-about way (via INotifyPropertyChanged), but it does work, and allows binding to that list safely using WPF.
Since the entire list is effectively updates, this will not suit all applications of course, where INotifyCollectionChanged may be desired interaction with the list. But for the purposes of this library, it allows binding to the list safely. Since the list is also now providing an underlying immutable list, it is even more thread-safe than before.