[DO NOT MERGE][NEXT?] Unsubscribe source observable immediately after it completes or errors

[mpodlasin]

Disclaimer:
For discussion. Either I am doing something very smart or something very dumb right now. ;)

Problem:
Currently many operators still subscribe for some time to source observables after they completed, and thus do not call teardown logic immediately after completion. These kind of marbles are in many tests:

---a-----b-----c-----|               <- source observable
^------------------------------!    <- subscription to that observable

It is a source of very subtle bugs, like: #2455

It also seems that this goes against tc39 proposal, which guarantees that source will be unsubscribed whenever it sends complete or error notification to an observer: https://tc39.github.io/proposal-observable/#subscription-observer-objects

There are operators that have to manually trigger their unsubscription to normalilze their behaviour: https://github.com/ReactiveX/rxjs/blob/master/src/operator/take.ts#L80

[kwonoj]

My personaly suggestion is starting for issue to discuss details first for these kind of cases, as it is not ceratin what desired behavior we want to have, this PR probably stall very long time and either be closed or required lot of updates in further.

[kwonoj]

(But not saying this should be closed now since it's already prepped)

[rxjs-bot]

	Warnings
⚠️	commit message does not follows conventional change log (1)

(1) : RxJS uses conventional change log to generate changelog automatically. It seems some of commit messages are not following those, please check contributing guideline and update commit messages.

Generated by 🚫 dangerJS

[coveralls]

Coverage increased (+0.004%) to 97.692% when pulling 270f2ad on Podlas29:subscriber-fix into 69d051b on ReactiveX:master.

[mpodlasin]

@kwonoj Will have that in mind, but I wanted to see if my planned fix will work and won't break stuff it shouldn't.

[kwonoj]

@Podlas29 if you don't mind, would you create issue? It's better to make discussion continues on issue, not on PR.

[trxcllnt]

@Podlas29 the spirit of the PR seems right, but allocating an extra Subscription per Subscriber shouldn't be necessary. We essentially hook up the "parent" subscription here, so I have a hunch we can update this PR to use that directly.

[mpodlasin]

@kwonoj #2459

[mpodlasin]

@trxcllnt Awesome pointer. Thanks! I was able to get rid of all unneccessary subscriptions.

[coveralls]

Coverage increased (+0.02%) to 97.707% when pulling 1c5282a on Podlas29:subscriber-fix into 69d051b on ReactiveX:master.

[coveralls]

Coverage increased (+0.02%) to 97.707% when pulling 1c5282a on Podlas29:subscriber-fix into 69d051b on ReactiveX:master.

[benlesh]

This PR will require a LOT of review. The behavior seems right-ish, on the surface, and the spirit of the PR seems solid. But there are a lot of questions and even more changes to review.

Most of our tests were modeled after the tests that existed in Rx4 and under. So many of these changes may incur breaking changes for the historical use of the library.

It also looks like (at a quick glance) that we're adding another Subscription reference per subscription, as well as a few new publicly available properties and methods.

Questions:

How does this affect performance?
How about memory pressure?
Who will this break?
Is this fixing a problem that many people are affected by? Is it really a problem?

There are just so many behavioral changes here, it's really going to be hard to tackle this and give it the proper attention it deserves in any reasonable amount of time.

[benlesh]

As an aside: the format of your commit messages is incorrect.

Perhaps it would be better to file each bug you think is a bug separately with it's accompanying test from this PR, so each one could be discussed individually?

[mpodlasin]

@benlesh - please take a look at #2459 In following days I will try to flesh out the problem more, provide more examples as well as alternative plans for fixing it in a more controlled/gradual way.

Also bear in mind I am 100% okay with closing this and fixing stuff on case-to-case basis. I just wanted to be sure that we are all aware of an issue. For me it's very fundamental weakness from conceptual standpoint, but I fully understand pragmatic approach.

[benlesh]

Okay, I think this PR should be redone and added as a bug fix during 6.0. It's not really a "breaking" change to anything but a broken behavior, IMO.

[benlesh]

Closing in favor of #3963, which is an attempt to pull this in.