fix: Disposables management for thread-safe operations #106
Conversation
|
Hello, to make the commit valid, you must rename it with the correct tag |
antoine-hebert
changed the title
|
Hi, can you provide benchmark results in your PR description for both before and after your change? |
|
|
||
| namespace Chinook.DynamicMvvm | ||
| { | ||
| public partial class ViewModelBase | ||
| { | ||
| private readonly Dictionary<string, IDisposable> _disposables = new Dictionary<string, IDisposable>(); | ||
| private readonly CancellationTokenSource _cts = new CancellationTokenSource(); | ||
| private readonly ConcurrentDictionary<string, IDisposable> _disposables = new(); |
There was a problem hiding this comment.
The way the library is currently designed doesn't imply thread safety. If we want to add thread safety as a feature, we would need to do more changes than just this one.
Extension methods such as TryGetDisposable, GetOrCreateDisposable, and their calling sites would need to be reworked with thread safety in mind because they currently have a naive implementation. (e.g. Checking if the dictionary contains an item and adding an item based on that condition is done in 2 steps rather than as an atomic operation.)
It makes sense to have thread safety in this library. However if we add it, it would need to be everywhere to be consistent.
GitHub Issue: #105
Proposed Changes
What is the current behavior?
Adding or removing a disposable could result in an InvalidOperationException in a muti threaded context.
What is the new behavior?
Enumerating disposables while adding or removing is now thread-safe.
Impact on version
buildfolder was changed..githubfolder was changed.Checklist
Please check that your PR fulfills the following requirements:
Other information