Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Many operators do not call teardown logic of source Observable immediately after it completes or errors #2459

Closed
mpodlasin opened this issue Mar 13, 2017 · 2 comments
Assignees
Labels
bug Confirmed bug

Comments

@mpodlasin
Copy link
Contributor

mpodlasin commented Mar 13, 2017

#2457 - general problem description and proposed solution.
#2455 - bug that made me discover an issue, directly caused by it. Contains much more in-depth explanation of what is happening in that concrete case.

Consequences:

  1. every take-ish operator (one that completes earlier than its source Observable), will keep subscribing to source Observable when combined with operator that prolongs subscription (here switchMap).
Rx.Observable.interval(1000)
.do(() => console.log('First just wanted one value, but I\'ll keep creating them forever'))
.first()
.switchMap(x => Rx.Observable.interval(1000))
.subscribe();
  1. custom made Observable (third party & via create) will not have their teardown logic called when they complete, when combined with operator that prolongs subscription (here switchMap):
Rx.Observable.create(obs => {
  obs.next(5);
  obs.complete();
  
  return () => console.log('I will never be called');
})
.switchMap(x => Rx.Observable.interval(1000))
.subscribe();
  1. Some error handling operators (currently only onErrorResumeNext) might still hold subscription to an Observable that had an error in its operator chain:
Rx.Observable.onErrorResumeNext(

  Rx.Observable.interval(1000)
  .do(x => console.log('a', x)) // this will be logging forever, even when map beneath errors
  .map(i => {
    if (i > 10) {
      throw new Error('oops!');
    }
  
    return i;
  }),

  Rx.Observable.interval(1000)

)
.do(x => console.log('b', x))
.subscribe();

What operators it affects?

Points 1. and 2. affect all operators that have following pattern in their unit tests:

----a----b----c----|                  <- Observable on which operator is used
^--------------------------!          <- subscriptions diagram for that Observable
---a-----b----c---|
^---------------------------

since | is the moment when Observable completes and ! is the moment when unsubscription function is called (which happens too late or sometimes does not happen at all).

Point 3. from what I have seen affects only onErrorResumeNext but in general it might affect all operators with following pattern in their unit tests:

----a----b----c----#
^--------------------------!
---a-----b----c---#
^---------------------------

since # marks the moment when Observable errored and ! marks the moment when unsubscription function is called.

Alternative fix:

  1. Force unsubscription in take-ish operators at the right moment (as matter of fact this is what take does: https://github.com/ReactiveX/rxjs/blob/master/src/operator/take.ts#L80). PRs: fix(first): force unsubscription logic on complete and error #2463 fix(takeWhile): force unsubscribe when it completes or errors #2470 fix(elementAt): force unsubscribe when it completes or errors #2501 fix(find): force unsubscribe when it completes or errors #2550
  2. ???
  3. ???
@benlesh
Copy link
Member

benlesh commented Apr 2, 2018

Okay, in order to resolve this, we need to figure out every path that has this issue, file separate bugs for those, then close this issue.

cartant added a commit to cartant/rxjs that referenced this issue Jul 27, 2018
cartant added a commit to cartant/rxjs that referenced this issue Jul 28, 2018
@cartant
Copy link
Collaborator

cartant commented Jan 27, 2019

Closing this as a related PR has been merged.

@cartant cartant closed this as completed Jan 27, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Feb 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Confirmed bug
Projects
None yet
Development

No branches or pull requests

3 participants