Fix order of observer and resource disposal of the Using and Finally operator

[quinmars]

This should fix #829. For the Using operator, I found a solution without adding extra allocations. I have no idea how that could be done for the Finally operator, hence one extra allocation.

[danielcweber]

What do you mean by "synchronous" case? If Run will observe termination of the source, why is it important to wait for Run to return before disposing the resource?

(I realize I haven't dealt with this for quite a while ;-))

[quinmars]

If the source is immediate terminated, _.Dispose() will be called before the upstream disposable is set. So the resource would (and currently will) be disposed in the sink's dispose method. Later when SetUpstream is called the source subscription will be disposed, due to the magic of "TryDispose/SetSingle". With this PR the TryDispose/SetSingle magic is used for both the source subscription and the resource -- in the correct order.

[danielcweber]

I might be missing it, but I can't see why the switch in Dispose alone won't already do.

Besides: Do we really guarantee a specific order of disposal? I don't think this is a breaking change. You should not rely on the order though, I guess...

[quinmars]

This was my first idea as well, but it only solved the second unit test (with the never terminating sequence). The switch does not help if the source sequence is terminated, before the subscription is passed to SetUpstream().

If you take

using (disposable1)
{
    using (disposable2)
    {
        ...
    }
}

disposable2 is guaranteed to be disposed before disposable1 under any circumstances. I would excpect the same for

Observable.Using(() => disposable1,
    _ => Observable.Using(() => disposable2,
        ...
));

and indeed that was the case in v4.0.

[quinmars]

Ok, I see. I will add that this evening, thanks. I will do the same for the Using operator, because I think in the current PR version it could happen that the resource disposal is started while the subscription disposal is still running.

[quinmars]

I changed the Finally implementation to not use an anonymous as @akarnokd suggested. So the current PR does not introduce any additional allocations.

Regarding my concerns with the Using operator, I play out some possible scenarious and I'm now very confident that the execution of the two Dispose methods will not overlap, hence I haven't changed that implementation.

[danielcweber]

Could you please provide a test for Finally as well?

I think the changes are fine, I just don't know whether we want to give any guarantees wrt. to the order of disposals. The recent performance enhancements have obviously changed some of the orders, there might be more occasions throughout the codebase. This PR's changes might definitely be desirable, but they also add some extra complexity. If we decide to guarantee a specific order from now on, we might run into even more issues like this down the road, and we might block ourselves from evolving the codebase.

I could use some opinions here.

[quinmars]

From a user perspective constant behavior is desirable, but I do understand your concerns about the lost flexibility and one can see with this PR that those guarantees lead indeed to extra complexity. I do not advocate to nail down the order of all disposals, with the Using and Finally operator, however, it is appropriate in my opinion.

The Using operator would be pretty useless if the dispoable resource could be disposed while it is still in used. Similar, a Finally callback should be invoked at the very end and not somewhere along the line.

[danielcweber]

Alright. If we become aware of more changes in behaviour from 4.0 to 4.x, we'll decide for each case. Thanks for the contribution!