3
\$\begingroup\$

I have project where I'm using BindableBase for my INotifyPropertyChanged implementation. Some of my objects however are generating a very rapid and bursty stream of INPC notifications, which is causing my UI to update unnecessarily often - to the point of making it laggy. To compensate, I'm attempting to throttle the speed at which these notifications are sent (e.g. once per 500ms for certain objects). Also, as part of this exercise, I've written unit tests for the throttling behavior, and I'd appreciate feedback on those as well.

The expected usage of the TaskScheduler dependency is that a free-threaded object (e.g. a model class), the BindableBaseWithThrottling could use TaskScheduler.Default (i.e. the ThreadPoolTaskScheduler) to allow continuations on a background thread, whereas a thread-affine object might use TaskScheduler.FromCurrentSynchronizationContext() or a custom scheduler to preserve thread affinity for the notifications. Also, it greatly simplifies controlling timing while running tests. :)

BindableBaseWithThrottling.cs:

using System;
using System.Collections.Concurrent;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;

namespace ThrottlingReview
{
    public abstract class BindableBaseWithThrottling : BindableBase
    {
        private readonly TimeSpan _throttleLimit;
        private readonly TaskScheduler _taskScheduler;
        private readonly ConcurrentDictionary<string, Task> _cooldownTasks;
        private readonly ConcurrentDictionary<string, string> _pendingNotifications;

        public BindableBaseWithThrottling(TimeSpan throttleLimit, TaskScheduler taskScheduler)
        {
            _throttleLimit = throttleLimit;
            _taskScheduler = taskScheduler;

            _cooldownTasks = new ConcurrentDictionary<string, Task>();
            _pendingNotifications = new ConcurrentDictionary<string, string>();
        }

        protected override void NotifyPropertyChanged([CallerMemberName] string propName = "")
        {
            if (_pendingNotifications.ContainsKey(propName))
            {
                // do nothing, it's already pending
            }
            else if (_cooldownTasks.TryGetValue(propName, out _))
            {
                // in cooldown, but not pending yet
                _pendingNotifications.TryAdd(propName, propName);
            }
            else
            {
                // fire right away if we're not already in a cool down
                base.NotifyPropertyChanged(propName);
                Cooldown(propName);
            }
        }

        private void Cooldown(string propName)
        {
            var newCooldown = new Task(async () => await Task.Delay(_throttleLimit));
            if (_cooldownTasks.TryAdd(propName, newCooldown))
            {
                newCooldown.Start(_taskScheduler);
                newCooldown.ContinueWith((t) => CheckForPendingNotifications(propName), _taskScheduler);
            }
        }

        private void CheckForPendingNotifications(string propName)
        {
            _cooldownTasks.TryRemove(propName, out _);

            // if there was a new pending request during the cooldown, service it now
            if (_pendingNotifications.TryRemove(propName, out _))
            {
                base.NotifyPropertyChanged(propName);
                Cooldown(propName);
            }
        }
    }
}

BindableBaseWithThrottlingTests.cs:

using System;
using System.Threading.Tasks;
using Xunit;

namespace ThrottlingReview
{
    public class BindableBaseWithThrottlingTests
    {

        [Fact]
        public void NotifyPropertyChanged_CalledOnce_FiresImmediately()
        {
            int count = 0;
            var scheduler = new DeterministicTaskScheduler();
            var sut = new TestBindableObject(TimeSpan.FromMilliseconds(1), scheduler);
            sut.PropertyChanged += (s, e) =>
            {
                count++;
            };

            sut.Value = 1; // trigger INPC notification

            Assert.Equal(1, count);
        }

        [Fact]
        public void NotifyPropertyChanged_CalledOnce_FiresOnce()
        {
            int count = 0;
            var scheduler = new DeterministicTaskScheduler();
            var sut = new TestBindableObject(TimeSpan.FromMilliseconds(1), scheduler);
            sut.PropertyChanged += (s, e) =>
            {
                count++;
            };

            sut.Value = 1; // trigger INPC notification
            scheduler.RunTasksUntilIdle();

            Assert.Equal(1, count);
        }


        [Fact]
        public void NotifyPropertyChanged_CalledTwiceImmediately_OnlyOneEventFiredImmediately()
        {
            int count = 0;
            var scheduler = new DeterministicTaskScheduler();
            var sut = new TestBindableObject(TimeSpan.FromMilliseconds(1), scheduler);
            sut.PropertyChanged += (s, e) =>
            {
                count++;
            };

            sut.Value = 1; // trigger INPC notification
            sut.Value = 2; // trigger after cooldown, but taskScheduler has not run any tasks, still in cooldown

            Assert.Equal(1, count);
        }

        [Fact]
        public void NotifyPropertyChanged_CalledTwiceImmediately_FiresSecondWithDelay()
        {
            int count = 0;
            var scheduler = new DeterministicTaskScheduler();
            var sut = new TestBindableObject(TimeSpan.FromMilliseconds(1), scheduler);
            sut.PropertyChanged += (s, e) =>
            {
                count++;
            };

            sut.Value = 1; // trigger INPC notification
            sut.Value = 2; // trigger INPC notification after cooldown
            scheduler.RunTasksUntilIdle();

            Assert.Equal(2, count);
        }

        [Fact]
        public void NotifyPropertyChanged_CalledThriceImmediately_FiresSecondWithDelay()
        {
            int count = 0;
            var scheduler = new DeterministicTaskScheduler();
            var sut = new TestBindableObject(TimeSpan.FromMilliseconds(1), scheduler);
            sut.PropertyChanged += (s, e) =>
            {
                count++;
            };

            sut.Value = 1; // trigger INPC notification
            sut.Value = 2; // trigger INPC notification after cooldown
            sut.Value = 3; // falls within 2nd notification
            scheduler.RunTasksUntilIdle();

            Assert.Equal(2, count);
        }


        [Fact]
        public void NotifyPropertyChanged_CalledTwiceWithDelay_FiresSecondImmediately()
        {
            int count = 0;
            var scheduler = new DeterministicTaskScheduler();
            var sut = new TestBindableObject(TimeSpan.FromMilliseconds(1), scheduler);
            sut.PropertyChanged += (s, e) =>
            {
                count++;
            };

            sut.Value = 1; // trigger INPC notification
            scheduler.RunTasksUntilIdle();
            sut.Value = 2; // trigger INPC notification after cooldown

            Assert.Equal(2, count);
        }

        [Fact]
        public void NotifyPropertyChanged_CalledWithDifferentPropertyNames_FiresImmediatelyForEach()
        {
            int count = 0;
            var scheduler = new DeterministicTaskScheduler();
            var sut = new TestBindableObject(TimeSpan.FromMilliseconds(1), scheduler);
            sut.PropertyChanged += (s, e) =>
            {
                count++;
            };

            sut.Value = 1; // trigger INPC notification
            sut.SecondValue = "test"; // trigger INPC notification for 2nd property name before cooldowns are run

            Assert.Equal(2, count);
        }

        private class TestBindableObject : BindableBaseWithThrottling
        {
            private int _value;
            private string _secondValue;

            public TestBindableObject(TimeSpan throttleLimit, TaskScheduler taskScheduler)
                : base(throttleLimit, taskScheduler)
            {
            }

            public int Value
            {
                get { return _value; }
                set { SetProperty(ref _value, value); }
            }

            public string SecondValue
            {
                get { return _secondValue; }
                set { SetProperty(ref _secondValue, value); }
            }
        }
    }
}

I'm using the DeterministicTaskScheduler described here to control execution flow of the cooldowns during tests. A full copy of all the files referenced (i.e. including BindableBase and DeterministicTaskScheduler) can be found in this gist.

Edit: responding to the CR feedback

  1. Race conditions: valid point. The two potential race conditions I could see are somehow getting multiple notifications to fire, or having one notification be put into pending but skipping the cooldown trigger - it wouldn't fire unless another trigger came through. For my use case, this isn't likely to be an issue, but it could be in other use cases.
  2. Thread pool consumption: Shouldn't be a problem. Creating task objects doesn't schedule them right away, and only one should ever get scheduled at a time (whichever one gets added to the _cooldownTasks collection).
  3. WPF Binding Delay property: this property affects the delay when a target updates the source in a two-way bind. I'm trying to throttle the rate at which the source updates the target, so it unfortunately doesn't help my case.
\$\endgroup\$
  • 1
    \$\begingroup\$ Your UI is WPF? jonathanantoine.com/2011/09/21/… \$\endgroup\$ – dfhwze Sep 10 at 16:36
  • 1
    \$\begingroup\$ @dfhwze Wow thanks I didn't even think to look for a solution via the binding. Well, this was a good learning exercise either way. ;) \$\endgroup\$ – Jimmy Sep 10 at 16:50
  • \$\begingroup\$ On further investigation, the Delay property doesn't help, since I'm in a one-way bind and need to limit the rate from the source. It was a good suggestion though. \$\endgroup\$ – Jimmy Sep 11 at 4:26
4
\$\begingroup\$

Race Conditions

You are using 2 concurrent dictionaries, which are each thread-safe, but used together there may be race conditions now and then. Think about possible impact and worst case scenarios.

_cooldownTasks = new ConcurrentDictionary<string, Task>();
_pendingNotifications = new ConcurrentDictionary<string, string>();

Thread Pool Resource Consumption

Since you expect massive amounts of updates, having a Task for each update might just put too much pressure on the thread pool:

var newCooldown = new Task(async () => await Task.Delay(_throttleLimit));

WPF Binding Delay

If you don't mind using a built-in solution (as we have established your UI is in WPF), you could go for a Delay on the Binding. Something like this:

Value="{Binding ElementName=ValueText, Delay=500, Path=Text, Mode=TwoWay}"
\$\endgroup\$
1
\$\begingroup\$

I would personally prefer a simpler approach, using System.Reactive (Rx.NET):

public abstract class ViewModel : INotifyPropertyChanged
{
    public ViewModel(int throttlingPeriod = 250)
        : this(TimeSpan.FromMilliseconds(throttlingPeriod))
    {
    }

    public ViewModel(TimeSpan throttlingPeriod)
    {
        Subject = new Subject<string>();
        Subject
            .GroupBy(pn => pn)
            .SelectMany(g => g.Sample(throttlingPeriod))
            .Select(pn => new PropertyChangedEventArgs(pn))
            .ObserveOn(SynchronizationContext.Current)
            .Subscribe(e => PropertyChanged(this, e));
    }

    public event PropertyChangedEventHandler PropertyChanged = delegate { };
    Subject<string> Subject { get; } 
    protected void RaisePropertyChanged([CallerMemberName] string propertyName = null) => 
        Subject.OnNext(propertyName);
}
\$\endgroup\$
1
\$\begingroup\$

An offline code review came up with a critical piece of feedback:

The constructor for Task takes an Action<T>. The async delegate therefore generates an async void method, which has 2 implications:

  1. If an unhandled exception is thrown from the async void method, it will crash the process (unlikely, since it's just calling Task.Delay(), but it's an unintended risk - especially if the code is changed/refactored later).
  2. The async void will return when it reaches the first await. An explanation on why is here. The continuation will then run almost immediately. The unit test coverage happened to miss this behavior because the continuation was scheduled on the DeterministicTaskScheduler, so it wasn't made apparent that the code wasn't actually waiting for the Task.Delay() to return. (The tests could be changed to not use RunTasksUntilIdle() since it hides how many tasks are scheduled then executed, and instead better inspect/execute the order that code was run.)

TL;DR: don't pass an async delegate to an Action<...> parameter. Instead find an overload that takes in Func<..., Task>, or implement the delegate as synchronous code.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.