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

Subscription to Observable is not removed from observers array after unsubscribe() #4230

Closed
ababashka opened this issue Oct 5, 2018 · 38 comments
Labels
bug Confirmed bug

Comments

@ababashka
Copy link

Bug Report

Current Behavior
Currently subscription isn't removed from observers array after unsubscribe, both with takeUntil() and unsubscribe().

Reproduction

function called() { console.log('CALLED'); }

const subject = new Subject();
const destroyed = new Subject();

const subscription = subject
        .pipe(takeUntil(destroyed))
        .subscribe(() => called());

console.log(subject.observers.length); // 1

subject.next(); // CALLED
subject.next(); // CALLED

// Unsubscribe from subject
destroyed.next(true);
destroyed.complete();

console.log(subject.observers.length); // Still 1, expected: 0

subscription.unsubscribe();
subject.next(); // no console.log('CALLED')

console.log(subject.observers.length); // Still 1, expected: 0

Expected behavior
In 2 last console.log(subject.observers.length) it's expected to have 0 : observer should be removed from array of observers after unsubscribe()

Environment

  • RxJS version:
  • 6.3.3

Additional notes
It isn't reproducible with rxjs version 6.2.2

@ababashka ababashka changed the title Subscription to Observable is not removed from **observers** array after **unsubscribe()** Subscription to Observable is not removed from observers array after unsubscribe() Oct 5, 2018
@mdepasquale21
Copy link

same issue here after upgrading from 6.3.2 to 6.3.3

@cartant
Copy link
Collaborator

cartant commented Oct 5, 2018

For me, this issue's repro outputs the expected counts:

$ ts-node index.ts
1
CALLED
CALLED
0
0

The code is unchanged:

import { Subject } from "rxjs";
import { takeUntil } from "rxjs/operators";

function called() { console.log('CALLED'); }

const subject = new Subject();
const destroyed = new Subject();

const subscription = subject
  .pipe(takeUntil(destroyed))
  .subscribe(() => called());

console.log(subject.observers.length); // 1

subject.next(); // CALLED
subject.next(); // CALLED

// Unsubscribe from subject
destroyed.next(true);
destroyed.complete();

console.log(subject.observers.length); // Still 1, expected: 0

subscription.unsubscribe();
subject.next(); // no console.log('CALLED')

console.log(subject.observers.length); // Still 1, expected: 0

And I'm testing it with 6.3.3:

$ npm list rxjs
[email protected] C:\Users\Nicholas\git\temp\so-rxjs-v6-scratchpad
`-- [email protected]

@ababashka
Copy link
Author

@cartant You are right, i've started new (angular) project and couldn't reproduce this issue with RxJS 6.3.3.
But still i can reproduce it here:
https://stackblitz.com/edit/rxjs-me3evv?file=index.ts

I've copied content of node_modules/rxjs/bundles/rxjs.umd.js bundle and put it into file rxjsbundle.js in my example on stackblitz.

Also, some additional note: i found this issue in my angular project's test and when i start this karma test with ng - it's reproducible. I've updated only rxjs in my package.json (from 6.2.2 to 6.3.3 as i mentioned previously) - and that failed.

Thanks for your time and would be nice if you could clarify this.

@cartant
Copy link
Collaborator

cartant commented Oct 6, 2018

So are you saying that it's only reproducible when using the bundle that's in 6.3.3?

@ababashka
Copy link
Author

Actually i've just tried to copy bundle's content to my example from versions 6.3.2, 6.3.1 and even from 6.2.2 - it's still reproducible with stackblitz and that's pretty strange.
But the test got failed only with RxJS version 6.3.3 and is working with 6.3.2

@cartant
Copy link
Collaborator

cartant commented Oct 6, 2018

This works fine for me, too:

<html>
<body>
<script src="./node_modules/rxjs/bundles/rxjs.umd.js"></script>
<script>

const { Subject } = rxjs;
const { takeUntil } = rxjs.operators;

function called() { console.log('CALLED'); }

const subject = new Subject();
const destroyed = new Subject();

const subscription = subject
    .pipe(takeUntil(destroyed))
    .subscribe(() => called());

console.log(subject.observers.length); // 1

subject.next(); // CALLED
subject.next(); // CALLED

// Unsubscribe from subject
destroyed.next(true);
destroyed.complete();

console.log(subject.observers.length); // Still 1

subscription.unsubscribe();
subject.next(); // no console.log('CALLED')

console.log(subject.observers.length); // Still 1

</script>
</body>
</html>

capture

@cartant
Copy link
Collaborator

cartant commented Oct 6, 2018

In your StackBlitz, you are using two separate implementations of RxJS: the bundle; and the package.json dependency. You are importing Subject from the former and takeUntil from the latter.

I suspect that the problem is directly related to the two implementations and that the changes made in 6.3.3 that have effected this behaviour are in 50ee0a7 and/or 0972c56.

The problem is reproducible with this code:

const { Subject } = require("rxjs/bundles/rxjs.umd");
import { takeUntil } from "rxjs/operators";

function called() { console.log('CALLED'); }

const subject = new Subject();
const destroyed = new Subject();

const subscription = subject
  .pipe(takeUntil(destroyed))
  .subscribe(() => called());

console.log(subject.observers.length); // 1

subject.next(); // CALLED
subject.next(); // CALLED

// Unsubscribe from subject
destroyed.next(true);
destroyed.complete();

console.log(subject.observers.length); // Still 1, expected: 0

subscription.unsubscribe();
subject.next(); // no console.log('CALLED')

console.log(subject.observers.length); // Still 1, expected: 0

And the problem does not occur if 6.3.2 is used.

@cartant cartant added bug Confirmed bug and removed status: more information needed labels Oct 6, 2018
@mdepasquale21
Copy link

mdepasquale21 commented Oct 9, 2018

In my case I have a working angular app (cli version 6.2.4, with rxjs 6.3.2) , in which I have a component behaving more or less like this:

import { Component, OnDestroy, OnInit } from '@angular/core';
import { Subscription } from 'rxjs';
import { tap } from 'rxjs/internal/operators';

export class MyComponent implements OnInit, OnDestroy {
    myObservable: Observable<T>;
    mySubscription: Subscription;

    ngOnInit() {
        console.log('subscription');
        this.mySubscription = this.myObservable.pipe(
            tap(() => {
                console.log('do stuff with observable');
            })
        ).subscribe(
            () => {
                console.log('do other stuff!');
            }
        );
    }

    ngOnDestroy() {
        console.log('unsubscribe');
        this.mySubscription.unsubscribe();
     }
}

After the update to rxjs 6.3.3, in the console I see:
> subscription
> do stuff with observable
> do other stuff!
> unsubscribe
> do stuff with observable

@cartant
Copy link
Collaborator

cartant commented Oct 9, 2018

@mdepasquale21 For this problem to manifest, I think you'd need to have multiple copies of RxJS installed in node_modules - either different versions or the same version in different locations.

Run npm list rxjs in your project's root to see what's installed.

@mdepasquale21
Copy link

mdepasquale21 commented Oct 9, 2018

@cartant you were right, shame on me! Some other dependencies in my app (@Angular-devkit), which depend in turn internally on rxjs, downloaded version 6.2.2 when npm installed, even if in their package.json they have rxjs: ''~6.3.0" (https://github.com/angular/angular-cli/blob/master/package.json). I tried also to delete node_modules and re-install everything, but looks like nothing changed. rxjs in my package.json was updated to 6.3.3, but version in @angular/cli was 6.2.2 so it was not the same everywhere, as you suggested. For some reason until version 6.3.2 this mixture did not break stuff!
I've tried a few solutions and it seems to me that it is very difficult to "force" the download of rxjs 6.3.3 for all dependencies, so actually I'm reverting to rxjs 6.2.2 to make everything work again. But I remain interested in future developments of this post!
Thanks!

@lowe0292
Copy link

Pretty sure this defect resulted in a huge memory leak across a bunch of our angular sites.

Reverting to rxjs 6.2.2 resolved the issue for us.

@danielgeri
Copy link

hi all - i am still a bit confused as to why there would be different version of rxjs installed for an angular application, especially when rxjs is listed as a peerDependency. I did a build w/o optimzation to check to see if there were multiple versions of rxjs installed and couldn't find more than 1. Also, when i run npm list rxjs, the version returned is 6.2.2; however when i look inside ./node_modules, I can clearly see that 6.3.3 is installed. I guess I would expect npm list rxjs to return 6.3.3? Admittedly, this is more of a how does npm work question rather than a rxjs question, but hoping someone here could enlighten me since it's related :). Thanks all!

@Spawnrad
Copy link

Spawnrad commented Oct 30, 2018

Same isssue here since i have update from 6.3.2 to 6.3.3 with angular-cli

@mdepasquale21
Copy link

@danielgeri I'm not sure either, the only thing I can suggest is that you try to delete node_modules directory and run npm install to reinstall everything from scratch. In my case when I tried this solution it didn't work. I had rxjs listed as a peer dependency but other libraries in my package.json which depend on rxjs for some reason were using a different version. For me it was completely unexpected, I had to revert to rxjs 6.2.2. Anyway after updating to Angular 7 everything works fine and I could finally use rxjs 6.3.3.

@Spawnrad
Copy link

Spawnrad commented Oct 31, 2018

I tried everything with deleting node_modules directory.

Angular 6 and rxjs 6.3.3 => not working
Angular 6 and rxjs 6.3.2 => working

Angular 7 and rxjs 6.3.3 => not working
Angular 7 and rxjs 6.3.2 => working

@jnizet
Copy link

jnizet commented Nov 19, 2018

FWIW, I've had the same issue when upgrading from 6.3.2 to 6.3.3, and the bug was in fact caused by an incorrect import: I imported from 'rxjs/internal/operators' instead of 'rxjs/operators'.

@cartant
Copy link
Collaborator

cartant commented Nov 19, 2018

@jnizet Do you have a minimal reproduction? I have an idea of how using the internal import location could be problematic, but I'd like to reproduce the problem. Mostly because I'm curious.

@jnizet
Copy link

jnizet commented Nov 19, 2018

I don't have a public one. I'll try creating one if I have some time, but it won't be now.

@mdepasquale21
Copy link

May be related with the discussion, I have an update with respect to my previous answer and in light of @jnizet 's answer: with angular 7 and rxjs 6.3.3 I thought everything was working fine, but instead I had a huge, random bug on initialization of the app! Sometimes the app just did not start, loading forever while sometimes started normally with no errors. So this 'init' bug appeared randomly without apparent reason whatsoever and it was difficult to reproduce and therefore to solve. I noticed I had several imports from 'rxjs/internal/operators' and changed them to import from 'rxjs/operators' as @jnizet suggested. This solved the problem finally (or at least it looks like, the bug has never appeared again since then).

@tiagowanke
Copy link

I have the same issue.
I downgraded to rxjs 6.3.2 and now is working.

@Spawnrad
Copy link

it looks like it work well with the new version 6.4.0. If someone can confirm ;)

@BenjaminBuick
Copy link

This issue is related to the pipe() operator - removing the operator before subscribe() restores functionality of the Subscription's object unsubscribe method.

I had this issue in combination with Angular 7. Using different versions (6.3.3 or 6.4.0 ...) had no effect. Angular versions are predeterming the rxjs version anyway.

@cartant
Copy link
Collaborator

cartant commented Feb 22, 2019

@BenjaminBuick If you have one, a reproduction of the problem would be appreciated. The comment you have left is unclear and not actionable, IMO.

@marianolop22
Copy link

Hi, same problem here
Angular CLI: 7.2.4
npm list rxjs
[email protected] C:\Desarrollo\Angular2\adminPro
+-- @angular-devkit/[email protected]
| +-- @angular-devkit/[email protected]
| | -- [email protected] deduped | +-- @angular-devkit/[email protected] | | -- [email protected] deduped
| +-- @angular-devkit/[email protected]
| | -- [email protected] deduped | +-- @ngtools/[email protected] | | -- [email protected] deduped
| -- [email protected] deduped +-- @angular/[email protected] | +-- @angular-devkit/[email protected] | | -- [email protected] deduped
| +-- @schematics/[email protected]
| | -- [email protected] deduped | -- [email protected]
| -- [email protected] deduped -- [email protected]

the code:

subscription: Subscription;

constructor() {
this.subscription = this.contarTresInfinito().subscribe(
(response: any) => {
console.log('respuesta', response);
},
(error) => {
console.error('hay error', error);

  }).add(
    () => {
      console.log('terminó')
  });

}

ngOnDestroy() {
this.subscription.unsubscribe();
console.log ('la pagina se va a cerrar');
}

public contarTresInfinito(): Observable {

return new Observable((observer: Subscriber<any>) => {

  let contador = 0;
  let intervalo = setInterval(() => {

    contador++;

    const salida = {
      valor: contador
    }

    observer.next(salida); //esto es lo que notifica

  }, 1000);

}).pipe(
  map(response => {
    return response.valor;
  }),
  filter((response, index) => { //el filter siempre devuelve un bool, el primer valor es el response y el segundo la cantidad de veces que es llamado

    if ((response % 2) == 1) {
      //impar
      return true;
    } else {
      //par
      return false;
    }
  })
);

it never unsubscribe.

@marianolop22
Copy link

well, i update the rxjs to 6.4.0, and then removed this line of the code
.add(
() => {
console.log('terminó')
});

now works perfect.

@hadirsa
Copy link

hadirsa commented May 2, 2019

I use angular 7.2.0 and rxjs 6.3.3 My problem is like this below. unsubscribe() has no effect and this.subscription.unsubscribe() in ngOnDestroy(){}.
is this problem persists?

@marianolop22
Copy link

I use angular 7.2.0 and rxjs 6.3.3 My problem is like this below. unsubscribe() has no effect and this.subscription.unsubscribe() in ngOnDestroy(){}.
is this problem persists?

do you have this line? .add() at the end of your subscribe?

@valentinabojan
Copy link

We faced the same issue with rxjs 6.3.3. After reading through all the comments here, we discovered that there was indeed a bad import of switchMap from rxjs/internal/operators. After fixing this import, everything works as expected.

@cartant
Copy link
Collaborator

cartant commented May 17, 2019

With the absence of minimal reproduction - and with reports that the problem is due to importing an internal file - IMO, this issue should just be closed.

@PaulAGH
Copy link

PaulAGH commented May 18, 2019

I also started encountering this after upgrading from Angular 6 to 7. On Chrome everything would behave, but on Firefox I would observe the following as the app would initialize.

Unhandled Promise rejection: You provided an invalid object where a stream was expected. You can provide an Observable, Promise, Array, or Iterable. ; Zone: ; Task: Promise.then ; Value: TypeError: "You provided an invalid object where a stream was expected. You can provide an Observable, Promise, Array, or Iterable."

I tried different versions of rxjs, examining closely npm list, etc - all to no avail.

Finally what did the trick was to change
import {Subject} from 'rxjs/index'; //This did not cause issues before the upgrade
to
import {Subject} from 'rxjs';

Crazy stuff!

It seems like import statements from 'rxjs/index' and 'rxjs/internal/*' (and perhaps others) should cause compilation errors.

@ztnark
Copy link

ztnark commented Sep 5, 2019

@PaulAGH I agree. This almost drove me mad :)

It seems like import statements from 'rxjs/index' and 'rxjs/internal/*' (and perhaps others) should cause compilation errors.

cartant added a commit to cartant/rxjs-tslint-rules that referenced this issue Sep 7, 2019
@cartant
Copy link
Collaborator

cartant commented Sep 7, 2019

I've added an rxjs-no-index rule to https://github.com/cartant/rxjs-tslint-rules/ and a no-index rule to https://github.com/cartant/eslint-plugin-rxjs. I would recommend using one of those rules to guard against explicit importation from an index module.

@cartant
Copy link
Collaborator

cartant commented Oct 25, 2019

I've made numerous requests for a minimal reproduction and - unless I've missed something - none has been provided. So this is not actionable, IMO.

It's possibly related to what #5059 will fix, but without a repro ... 🤷‍♂

@char-m
Copy link

char-m commented Oct 28, 2019

I've made numerous requests for a minimal reproduction and - unless I've missed something - none has been provided. So this is not actionable, IMO.

It's possibly related to what #5059 will fix, but without a repro ... 🤷‍♂

I questioned closing the issue on false premise. Sorry about confusion.

@JanEggers
Copy link

Im having the same issue. looking at the code I wonder why

unsubscribe(): void {
if (this.closed) {
return;
}
this.isStopped = true;
super.unsubscribe();
}

does not this.destination.unsubscribe()

@cartant
Copy link
Collaborator

cartant commented Oct 29, 2019

As I've said above:

I've made numerous requests for a minimal reproduction and - unless I've missed something - none has been provided. So this is not actionable, IMO.

Seriously, no one is going to be able to answer any questions about this without a minimal reproduction of the problem.

@melroy89
Copy link

Any news?

@cartant
Copy link
Collaborator

cartant commented Mar 15, 2021

Closing this because it cannot be reproduced.

@cartant cartant closed this as completed Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug
Projects
None yet
Development

No branches or pull requests