[BUG] DelayChangeNotifications does not delay PropertyChanged events

[WalkerCodeRanger]

Describe the bug
The ReactiveObject.DelayChangeNotifications() method does not delay the firing of PropertyChanged events. (Please note ReactiveObject.SuppressChangeNotifications() does suppress them.)

Steps To Reproduce
The following C# console app demonstrates the bug. All assertions are expected to pass, but 2 of 3 fail.

using System.Diagnostics;
using ReactiveUI;

class Example : ReactiveObject
{
    private int _value;

    public int Value
    {
        get => _value;
        set => this.RaiseAndSetIfChanged(ref _value, value);
    }
}

class Program
{
    static void Main(string[] args)
    {
        var obj = new Example();
        var changeEvents = 0;
        obj.PropertyChanged += (s, e) => changeEvents++;
        using (obj.DelayChangeNotifications())
        {
            obj.Value = 1;
            obj.Value = 2;
            Debug.Assert(changeEvents == 0, "Property change raised during delay");
        }
        Debug.Assert(changeEvents < 2, "Property change not deduped");
        Debug.Assert(changeEvents != 0, "Property change not raised");
    }
}

Expected behavior
Similar to how ReactiveObject.SuppressChangeNotifications() affects property changed events and ReactiveObject.DelayChangeNotifications() delays and removes duplicate change events from the ReactiveObject.Changed observable. I expect:

1. No property changing and changed events will be raised between the call of ReactiveObject.SuppressChangeNotifications() and the disposing of the returned IDisposable.
2. When the IDisposable is disposed of, any delayed changing and changed events will be raised
3. Changing and changed events that are delayed will be deduplicated so that if a property is changed more than once during the delayed time, only one property change event will be raised for it.

Environment

• OS: Windows Pro
• Version 10.0.18363
• Device: Dell Vostro 7590
• ReactiveUI: v11.3.1
• .NET Framework: v4.7.2

[WalkerCodeRanger]

Would you accept a pull request fixing this? (If I have time to make one)

[glennawatson]

Sure I would love a pr if you have time. Thanks for checking with us first.

[WalkerCodeRanger]

@glennawatson I was working on fixing this bug and I have a question about the best way to implement it and what you guys would consider to be an acceptable change, esp. considering how core this is.

In the interest of making sure my PR will be accepted, which of these options do the project maintainers find acceptable?

The ReactiveObject.DelayChangeNotifications() works correctly for the Changing and Changed observables. That all is somewhat complex because of deduplicating the change notifications and the understandable desire to not instantiate the observables unless they are needed.

Given that, there are two ways the fix for this bug could be implemented:

1. Piggyback on the observables, raise the events from the observables.
2. Duplicate a lot of the observables logic/code for use in raising the events.
   a. Using observables
   b. Using lists as buffers

Option 1: The easiest way to make the delay change notifications work for the PropertyChanging/ PropertyChanged events would be to subscribe to the observables and raise those events. However, this will change the exception handling for exceptions thrown from event handlers for those events. Currently, such exceptions will be thrown from the RaisePropertyChanging/RaisePropertyChanged events. That will prevent the change from ever being sent to the observable. If the code were changed to raise the events from the observable, then exceptions would be logged and recorded in the ThrownExceptions observable (see IReactiveObjectExtensions.NotifyObservable). I'm not sure what effect that has on the other subscribers. I'm guessing what order they subscribed in determines it.

Option 2a: Create separate code for delaying and deduping PropertyChanging/ PropertyChanged events. The most straightforward way to do that would be to create separate observables to pipe those through and raise events from. That would be sure to mirror the behavior of the other but hopefully allow for exception handling to be unchanged. However, if we want to delay the creation of those observables, the add/remove event accessors would have to be customized to create the observables the first time the events were wired up.

Option 2b: When PropertyChanging/ PropertyChanged events are delayed, create a list that buffers them until the end of the delay at which point they are deduped (can use the same code I think) and raised. This has the advantage of being straightforward, but also that it is practically zero overhead until events are actually delayed. Only when events are delayed do the buffers need to be created and additional work is done.

[glennawatson]

I think option 1 is out due to the different exception handling.

I personally like 2b myself

[RLittlesII]

I like 2b as well

[WalkerCodeRanger]

When you say "threads stop processing" are you just meaning because those threads are deadlocked they aren't processing or is this something else?

[jay1891]

Hi, I don't know if this is valuable information, but if it could help; The DelayChangeNotifications in the new version introduced thread deadlocks in our application. We use DelayChangeNotifications intensively in our core logic when we update our core domain state, which is logic that is monitoring in realtime real world object activity. We reversed back to the previous version and the problem didn't occur in the past 2,5 days. Problably it was not a good design decision to use DelayChangeNotification in our core domain, so we are removing it.

[WalkerCodeRanger]

@jay1891 It would be helpful if you could provide more detail about what it takes to reproduce this issue and file it as a separate bug. There is nothing in the code changes that jump out to me as likely to cause deadlocks. Perhaps it is just a subtle change in behavior that interacts with the rest of your code to cause deadlocks? Without more detail, it is unlikely anyone will address the issue.

[jay1891]

@WalkerCodeRanger, I think travbid filed a bug for this #2414

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.