fix: finalize after unsubscription from source

[cartant]

Description:

This PR changes the implementation of finalize so that the callback passed to the operator is not called until after the subscriber unsubscribes from its source. The effect of this is that - with this change - finalization happens in same order as completion and unsubscription.

Specifically, with unsubscription, we can be sure that when a subscription's unsubscribe method returns, every source observable in the chain will have been guaranteed to have been unsubscribed.

Without this change, finalize's behaviour is very different to that of unsubsciption.

The problem is that when finalize calls its callback, the observable to which it is applied is still subscribed to any sources and any finalize operators applied to sources further up the composed chain will not have called their finalization callbacks.

Aside from this behaviour being surprising, it makes finalize pretty much useless for resource management. For example, if there are observables that use and release some resource:

const a = /* whatever */.pipe(finalize(() => /* release some sub-resource */));
const b = /* whatever */.pipe(finalize(() => /* release some sub-resource */));

and are composed into another observable:

const c = merge(a, b).pipe(finalize(() => /* release the resource */);

the finalize calls will not do what you would expect, as the calls to release the sub-resources in a and b will happen after the release of the resource itself occurs in c and - depending upon the resource - that can effect an error.

Why the call order is weird without this change:

The order in which the callbacks are called is weird because the finalize operator adds the callback to the Subscription:

rxjs/src/internal/operators/finalize.ts

Lines 80 to 85 in dfa239d

	class FinallySubscriber<T> extends Subscriber<T> {
	constructor(destination: Subscriber<T>, callback: () => void) {
	super(destination);
	this.add(new Subscription(callback));
	}
	}

It's added in the Subscription constructor, so the callback is the first teardown in the subscription. The subscription to the source is going to be added after that:

rxjs/src/internal/operators/finalize.ts

Lines 70 to 72 in dfa239d

	call(subscriber: Subscriber<T>, source: any): TeardownLogic {
	return source.subscribe(new FinallySubscriber(subscriber, this.callback));
	}

rxjs/src/internal/Observable.ts

Lines 212 to 222 in dfa239d

	const sink = toSubscriber(observerOrNext, error, complete);
	if (operator) {
	sink.add(operator.call(sink, this.source));
	} else {
	sink.add(
	this.source || (config.useDeprecatedSynchronousErrorHandling && !sink.syncErrorThrowable) ?
	this._subscribe(sink) :
	this._trySubscribe(sink)
	);
	}

It's a breaking change:

Although the re-ordering of the finalize calls broke none of the existing tests, this is a breaking change, but it's a change for the better, IMO.

Related issue (if exists): See #5372 and issues linked therein.

[cartant]

I added this test as another example. I doesn't make much sense to me for the finalize callback to be called before the source observable is torn down.

[cartant]

@benlesh

The fix for this should be as simple as moving the addition of the callback to the operator call instead of in the constructor.

Yeah, that's much better. Thanks.