7

I'm running into a scenerio where subscribe fires 2x. However if I get rid of the service by copy pasting the poller method into my component then the issue goes away.

I am running in Ionic 3, but I don't think that matters here. "rxjs": "5.4.3"

unsubscribe() is called on ngDestroy()

my-service.service.ts

public pollingDataReceived = new Subject<any>();

pollForData = (id: string) : Subscription => {
    let stopPollingForStatus = false;
    return Observable.interval(environment['pollingInterval'])
          .takeWhile(() => !stopPollingForStatus)
          .concatMap(() => {
            return this.getAnalysisById(id);
          })
          .subscribe((analysisData) => {
            if (analysisData) {
              if (analysisData.status === 'questions') {
                stopPollingForStatus = false;
              }
              else {
                stopPollingForStatus = true;
                const result = {
                  someData: 'whatever'
                };
                console.log('raise pollingDataReceived event');// This happens ONCE
                this.pollingDataReceived.next(result);
              }
            }
            else {
              alert('no analysis data!');
            }
          });
  }

my.component.ts (a subscriber)


  private pollingData: Subscription;

  someEventHandler() {
    this.pollingData = this.pollForData(this.id);
  }

  ngOnInit(): void {
    this.myService.pollingDataReceived.subscribe((data) => {
        this.analysis = data.analysisData;
        //This is getting called 2x.  Maybe I double subscribed?
        myNavigator.navigateToPageByStatus(myPage);
    }, (err) => { console.log(err); });
    }

  ngOnDestroy() {
    console.log('ngOnDestroy: unsubscribe pollingData in flow');// This does get called
    this.pollingData.unsubscribe();
  }
kos
  • 5,044
  • 1
  • 17
  • 35
P.Brian.Mackey
  • 43,228
  • 68
  • 238
  • 348
  • How your service injected, means is it provided in the module or in component level? When you revisit it will create an instance of that service and initialize all the subscription once again because you have injected via constructor of that component and it will destroy only when you change your route to load another component. – Niraj Apr 09 '19 at 13:19
  • The service is injected into the component via the constructor. – P.Brian.Mackey Apr 09 '19 at 13:26
  • 1
    If you want you can only one time to call the subscriber something like this. `.pipe(take(1)).subscribe()` it will take only once. – TheCoderGuy Apr 09 '19 at 13:30
  • Just move to provide the service instance at module level to achieve singleton instance of your subscription resides in the service and ngondestory() in component will handle that subscription to unsubscribe and no more listening for that stream – Niraj Apr 09 '19 at 13:45
  • @P.Brian.Mackey - Where is the service provided? – ConnorsFan Apr 09 '19 at 13:57

7 Answers7

4

Angular Services behave like Singletons unless provided at a component level. Hence the pollingDataReceived Subject will continue to hold its subscriptions until the service is in scope.

In your case, the stale subscription to this.myService.pollingDataReceived from from the earlier visit also is getting fired. Hence the subscriber firing twice on revisit.

Cleaning it up on ngDestroy similar to pollingData will fix the issue.

ashish.gd
  • 1,713
  • 1
  • 14
  • 25
3

The reason for handler firing twice — is that you never unsubscribe from the service pollingDataReceived Subject, while you push to this subject multiple times. Your service was likely created once, while your components were recreated multiple times with page revisits. Components were destroyed, though their subscriptions to the same Subject remained intact.

Simplest way to workaround that is to unsubscribe from this subject in component ngOnDestroy method, as @ashish.gd suggested.

Creating new Subjects each time (as suggested here) will probably lead to memory leaks. As your old subscriptions will keep listening to old Subjects. Beware.

While the simplest way would be to unsubscribe from pollingDataReceived Subject, the better way would be to have your service pollForData() to return an Observable.

Unless you need several consumers to read the same pollingDataReceived Subject — you don't need to have a subscription there. Heres a sample code to reflect that idea:

pollForData = (id: string) : Observable => {
  return Observable.interval(environment['pollingInterval'])
    .concatMap(() => this.getAnalysisById(id))
    .takeWhile(data => data.status === 'questions')
    // other handling logic goes here
    // .map(), .switchMap(), etc
    // NOTE: no subscription here
}

and in the controller you could do something like

// just an utility subject to track component destruction
onDestroy$ = new Subject();

someEventHandler() {
  this.pollForData(this.id).pipe(
    // this will ensure that subscription will be completed on component destruction
    takeUntil(this.onDestroy$)
  ).subscribe((data) => {
      this.analysis = data.analysisData;
      myNavigator.navigateToPageByStatus(myPage);
  }, (err) => { console.log(err); });
}

ngOnDestroy() {
  this.onDestroy$.next(void 0);
}

--

NOTE 1: seems like currently your service is provided at module level, which means that if you'll ever have two consumers requesting pollForData(id) at the same time (e.g. two components on one page) — both of these requests will "write" into the same Subject. Error-prone behavior.

NOTE 2: currently in your code takeWhile predicate wont be run until you get a new event on the stream

Observable.interval(environment['pollingInterval'])
  .takeWhile(() => !stopPollingForStatus)

Which means that you'll have an unnecessary subscription in memory for environment['pollingInterval'] time. This should not be critical, yet it might be troublesome depending on timing.

--

Hope this helps, happy coding!

kos
  • 5,044
  • 1
  • 17
  • 35
  • @P.Brian.Mackey, considering what I mentioned in NOTE 1, I'd suggest two alternative solutions for sharing observables amongst several consumers: **1)** have a root component that will request data and then pass it as a property to child components (you'll have to use [`share`](https://observable-playground.github.io/rxjs/share/) for this) **2)** create a [WeakMap](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap) of Subjects in your service to store Subjects for different ids. – kos Apr 09 '19 at 19:06
  • @P.Brian.Mackey, actually, heres a 3rd approach. It will fit you only if you need to display same data for same ID at a given time. Note that it uses [`switchMap`](https://observable-playground.github.io/rxjs/switchMap/) when new data is requested — so data for previous IDs wont mess with new IDs data. Also, it exposes only an Observable, not a Subject. Check this approach here: https://stackblitz.com/edit/angular-y2ban3 – kos Apr 09 '19 at 20:03
2

I faced a similar problem, Try below code, which is just a workaround for the underlying problem.

this.pollingDataReceived.next(result);
this.pollingDataReceived = new BehaviorSubject<any>(); // Here, create new object with null value
Prashant Pimpale
  • 10,349
  • 9
  • 44
  • 84
  • 1
    I don't know why we would have to do this? Feels like rxjs is bugged. Note in my case it's `this.pollingDataReceived = new Subject();` – P.Brian.Mackey Apr 09 '19 at 13:49
  • @P.Brian.Mackey exactly, I am unable to find the root cause behind this but this workaround saved me! – Prashant Pimpale Apr 09 '19 at 13:51
  • 1
    @P.Brian.Mackey This will probably lead to memory leaks, see my answer for details. Rxjs is not bugged (well, generally :) ) it just takes time to wrap your head around it. You need to understand and be responsible for when and how your Observables are subscribed and completed. And the less subscriptions you have — less you have to worry about. GL – kos Apr 09 '19 at 15:26
  • Don't do this, unsubscribe from your subscription. Or better, learn about using the async pipe in your template rather than subscribing. – Adrian Brand Apr 10 '19 at 00:11
  • @AdrianBrand Will keep in mind, I updated my answer – Prashant Pimpale Apr 10 '19 at 05:06
  • How have you updated your answer? You are still abandoning an observable. At least call complete on it before you abandon it to clean up any subscriptions. This is a horrible hack to solve not unsubscribing properly. – Adrian Brand Apr 10 '19 at 07:46
  • @AdrianBrand if I unsubscribe, I get unscubscribed error the next time I open the component. After hours of trying this is the first answer that worked for me. – Balazs F. May 19 '20 at 19:37
1

Use a finalise subject and take until

  private pollingData: Subscription;

  private finalise = new Subject<void>();

  someEventHandler() {
    this.pollingData = this.pollForData(this.id);
  }

  ngOnInit(): void {
    this.myService.pollingDataReceived.pipe(
      takeUntil(finalise)
    ).subscribe((data) => {
        this.analysis = data.analysisData;
        //This is getting called 2x.  Maybe I double subscribed?
        myNavigator.navigateToPageByStatus(myPage);
    }, (err) => { console.log(err); });
    }

  ngOnDestroy() {
    this.finalise.next();
    this.finalise.complete();
  }
Adrian Brand
  • 20,384
  • 4
  • 39
  • 60
  • I think your answer is the most clean solution for housekeeping multiple subscriptions. Thank you for sharing and also editing the accepted incorrect answer. – ashish.gd Apr 10 '19 at 03:31
1

There is also another way to achieve this. You can use the ReplaySubject from rxjs.

class pollingComponent {
  private destroyed$: ReplaySubject<boolean> = new ReplaySubject(1);

  constructor(
    private pollingService: Service) {}

  ngOnInit() {
    this.pollingService
      .takeUntil(this.destroyed$)
      .subscribe(...);
  }

  ngOnDestroy() {
    this.destroyed$.next(true);
    this.destroyed$.complete();
  }
}

Basically it is advised to avoid vanilla Subject. You can read more about the difference here

Arghya Saha
  • 5,599
  • 4
  • 26
  • 48
  • I just want to point out that this solution deliniate exactly where does destroyed$ comes from. Previous answers bit me since I thought its was a built in event. – Mark Odey Feb 24 '21 at 16:31
0

If you need the event to stay subscribed while in you are visiting other components (like for messaging) you can subscribe only one time by setting a condition to check if there are no observers to the event and only then subscribe to it. I believe this is not best practice but it does the job.

It would look like this:

ngOnInit(): void {
      if (this.myService.pollingDataReceived.observers.length === 0) {
      this.myService.pollingDataReceived.subscribe((data) => {
          this.analysis = data.analysisData;
          myNavigator.navigateToPageByStatus(myPage);
      },  (err) => { console.log(err); });
    }
  }
0

The selected answer didn't work for me instead it triggered subscribe once again.

Unsubscribing BehaviorSubject is correct choice.

ngOnDestroy() {
    this.pollingData.unsubscribe();
  }

BehaviorSubject unsubscribe event triggers property updates but won't get reinitialized. Instead use closed property.

Before initializing again wrap it with below condition and then call next

if(this.pollingData.closed) {
  this.pollingData = new BehaviorSubject<any>(false); 
}
this.pollingData.next(res);

This worked perfectly and prevents triggering multiple subscribe events. To know about properties check out this answer

If Subject gets completed:

closed: false
isStopped: true

If unsubscribed:

closed: true
isStopped: true
Praveen
  • 55,303
  • 33
  • 133
  • 164