Dispose after removal, with .DisposeMany()

[JakenVeina]

During debugging of a disposal issue that turned out to be related to something else, I noticed that .DisposeMany() does not behave the way I thought it would: items being removed from collections get disposed by the operator before they are removed from downstream collections, instead of after, making it potentially possible for already-disposed items to be invoked in some way. This is due to the use of .Do() within .DisposeMany().

DynamicData/src/DynamicData/Cache/Internal/DisposeMany.cs

Line 34 in b3180bc

var subscriber = _source.Synchronize(locker).Do(changes => RegisterForRemoval(changes, cache), observer.OnError).SubscribeSafe(observer);

.Do() effectively amounts to...

public static IObservable<T> Do<T>(this IObservable<T> source, Action<T> onNext)
    => Observable.Create<T>(observer => source.Subscribe(value => 
    {
        onNext.Invoke(value);
        observer.OnNext(value);
    });

Instead, for .DisposeMany() it could be...

public static IObservable<T> Do<T>(this IObservable<T> source, Action<T> onNext)
    => Observable.Create<T>(observer => source.Subscribe(value => 
    {
        observer.OnNext(value);
        onNext.Invoke(value);
    });

This would ensure that disposal doesn't occur until after all downstream collections have processed the removals (at least assuming there are no downstream scheduler changes, which would potentially break .DisposeMany() no matter what).

[dwcullop]

You have the problem and the solution .... Now all you need is a unit test or two and you'll have yourself a PR!

[RolandPheasant]

@JakenVeina I think your observation that objects should be disposed after they have been removed it correct. It makes sense. I remember years ago getting a disposed object access exception but in that case the screen had a long delay on the UI before the object would be removed ie before being removed the row would be highlighted for a couple seconds then removed. My solution at the time was to do the dispose on a delayed stream, but that was probably a different use case.

The suggestion by @dwcullop would be an easy and probably reliable solution.

[dwcullop]

@JakenVeina I don't think this is a breaking change, I think it's more of a bug fix, because, as you pointed out, if anything downstream wants to perform some action on the object being removed, they're going to be doing it on a Disposed object, and it might be too late at that point.

I think I even hit that bug before, but I just worked around it by moving the DisposeMany to be AFTER the OnItemRemoved operator, but that might not always be possible.

[JakenVeina]

On a technical level, it's an observable (in the colloquial sense) change in the behavior or an API, so it's breaking.

On a practical level, yeah, I struggle to think of a scenario where it matters. It's a little unique on account of being about disposables, because the only way you could "observe" this change that makes disposal occur later is if you were previously using disposables after they'd been disposed. The change is that tomorrow, they might not be disposed yet, when they are today.

Is that sufficient for classifying the change non-breaking? I don't know what's normally the standard to apply, I've never maintained versioning for a large-scale public library.

[dwcullop]

Well, Hyrum's Law always applies and every change breaks someone's workflow, but I honestly think it is acceptable in this case. There might be places where people have worked around this issue and no longer need to, but I am having a hard time coming up with a situation where something would be broken because the Dispose happens later instead of sooner. As long as it still happens.

I that sufficient for classifying the change non-breaking? I don't know what's normally the standard to apply, I've never maintained versioning for a large-scale public library.

It's up to @RolandPheasant and he said he's fine with it. I think you're all clear to make a PR...

[RolandPheasant]

Although this would be a behavioural change, I regard it a bug fix and therefore a minor change, and cannot see that it will break any code.

Normally I try to apply semantic versioning which is roughly means {major}.{feature}.{minor} where:

major = this is going to break people
feature = new feature
minor = bug fix or optimisation

However as per this discussion there are grey areas but I always balance how likely would problems be caused for consumers.