5

I have this little bit complex pipe structure in my Angular + RxJS app:

remote-data.service.ts:

getAll(url): Observable<any[]> {
  return this.http.get(url, this.options)
    .pipe(map((result: any[]) => result));
}

proxy.service.ts:

// localDataService use my LocalDataService
// remoteDataService use my RemoteDataService

getAll(model): Observable<any[]> {
  if (model.use_localstorage) {
    return this.localDataService.getAll(model)
      .pipe(map((result: any[]) => result));
  } else {
    return this.remoteDataService.getAll(model.url)
      .pipe(map((result: any[]) => result));
  }
}

helper.service.ts:

getAll(model): Observable<any[]> {
  // do some general fancy things...
  return this.proxyService.getAll(model)
      .pipe(map((result: any) => result));
  }
}

Then in my component:

export class MyComponent {
  helperService = new HelperService();
  model = new MyModel();

  getAll() {
    this.helperService.getAll().subscribe((result: any[]) => {
      // parse result
    });
  }
}

As you see I build a pipeline from remote data service, proxy service, helper servcice and the component. Of course the reason is to separate each functions from eachother and make my services more reuseable.

My goal is to avoid the memory leaks.

And the question is: if I want to put a take(1) RxJS operator into my pipeline it's enough to put it into the end of pipeline, before the .subscribe() or need to put it every services and component too?

What is the best practice in this case to avoid memory leaks?

netdjw
  • 5,419
  • 21
  • 88
  • 162

2 Answers2

6

It depends. pipe(take(1)) ensures you get a "single Value observable" meaning, that it will emit one value then completes, and closes the subscription, hence no memoryleak. If your Service or function per se emits only a Single value pipe(take(1)) does nothing.

For Example if you have a REST call, with httpclient.get(...) it may have a delay. Then you should use pipe(timeout(3000)) or pipe(takeUntil(...)) to make sure to not have a subscritpion alive, if your component or whatever is destroyed you don't have a memory leak or unexpeced behaviour of the logic inside the subsription. Even with pipe(take(1)) a memoryleak can exist because it will only complete after exactly one value or error is emitted.

So if you have a http.get() and it has a network delay. And you would use "pipe(take(1))" it would still cause a memory leak because it waits for exactly one value, and the subscription will be triggered when the value arrives, even if the Component in wich you made the call is destroyed or you have navigated to an other View of your application.

takeUntil(...) is usefull for components, if they are destroyed you could trigger the ending of a subscription in ngDestroy().

  public isActive = new Subject();

  public ngOnDestroy(): void {
    this.isActive.next(false);
  }

  public fun():void{
    this.fooService.getValue()
                     .pipe(takeUntil(this.isActive))
                     .subscribe( value => console.log(value));
  }

If the Observable/Subject completes there should be no memoryleak after the last value is emitted.

You need to take care of those cases, if you are not sure, there is a "complete()" or if it emits only One value but the latency could be problematic.

The memoryleaks are btw the secondary problem, and not even that big. MOre problematic is, that the logic of your subscribe could be triggered, when you don't want to anymore. Sometimes you want to trigger the logic in the subscription (for example a notification to tell the user something was successful) even though the user already left the view. So it depends.

Also you could store All subscritions in on Single Subscription and unsubscribe if you want to dispose them.

  public subs = new Subscription();

  public ngOnDestroy(): void {
    this.subs.unsubscribe();
  }

  public fun():void{
    const bar: Subscription = this.fooService.getValue()
                     .subscribe( value => console.log(value));
    this.subs.add(bar);
  }
Luxusproblem
  • 1,913
  • 11
  • 23
  • So you say the HttpClient's calls don't occurs memory leaks, because they call `complete()` on every case. It's a good news to me, but maybe a bad news too, becasue my software's slowing down is coming from another reason... – netdjw May 31 '20 at 14:50
  • 1
    @netdjw I would suggest taking a look at this : https://stackoverflow.com/questions/35042929/is-it-necessary-to-unsubscribe-from-observables-created-by-http-methods/60480100#60480100 And no. There can be memoryleaks with httpClient. For the amount of time of the latency of your network. Wich could be a problem. Also if you have a latency case, there can be problems of your logic inside the subscribe, if you leave the view. For example routing, you leave the view because the network is slow. and 1 minute later, you get rerouted, because the response arives and triggers the logic. – Luxusproblem May 31 '20 at 17:46
  • I handle the network latency with a fullscreen overlay div. It is over all things, and spinner is animating while the network communication isn't finished. So the user can't do anything until the nework communciation isn't finished. Maximum he can reload the page, but this leads to the problem being eliminated. So the user can't leave the view until the network communication is in progress. But okay. I understand you. – netdjw Jun 02 '20 at 09:46
  • 1
    My final conclusion is: I need to use `take(1)` or `takeUntil(condition)` in every `pipe()` all around of my codes. – netdjw Jun 02 '20 at 09:48
  • 1
    @netdjw you only need `take(1)` if you know that there will be more then one value, but you only want one. If it is a httpClient rest call, take(1) does nothing, since REST Responses are already handled in Single Value Observables. TakeUtil/takeWhile are probably the best ways to deal with delayed arrival of values. And no you don't have to use it everywhere. It depends on what you do in your "subscribe" most of the times, its not that big of a deal, and won't do anything. But at critical operations in the subscribe (login, sensible data, rerouting) you should take care of it – Luxusproblem Jun 02 '20 at 11:09
3

There's a common pattern - takeUntil(this.destroy$) should be the latest operator in any pipe before .subscribe call.

class MyClass {
  destroy$ = new Subject();

  ngOnInit() {
    this.stream$.pipe(
      takeUntil(this.destroy$),
    ).subscribe(data => {
      // some logic.
    });
  }

  ngOnDestroy() {
    this.destroy$.next();
    this.destroy$.complete();
  }
}

There's a very nice explanation of all issues and how to solve them to avoid memory leaks: https://blog.bitsrc.io/6-ways-to-unsubscribe-from-observables-in-angular-ab912819a78f

satanTime
  • 12,631
  • 1
  • 25
  • 73
  • According to good practices here https://medium.com/better-programming/rxjs-best-practices-7f559d811514, it is better to avoid logic in subscribe(), instead, use pipe() to have reactive programming style instead of imperative programming. Even if your code works well, you should at least consider it :) – Moff452 Feb 16 '21 at 15:11