0

I have one stupid question maybe: I'm implementing some routes guards that contains some other observable subscription, they are provided in root, so they should be singleton, no need to unsubscribe really. But what I'm doing is returning all the time a new Observable that I completes manually (not sure if this is required), the internal subscription are cleared each execution with the private _sub field.

Question is: is this code safe from memory leaks (how is routing guard subscribed internally to this observable)? there is a recommended way to test and see if there are observables not unsubscribed in one application ?

@Injectable({
   providedIn: 'root'
})

export class MyRedirectGuard implements CanActivate {
   private _sub: Subscription;

   constructor(private myService: MyService,
               private router: Router) {
   } 

canActivate(next: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable<boolean | UrlTree> {
  if (this._sub)
     this._sub.unsubscribe();

       return new Observable<boolean | UrlTree>((observer: any) => {

        this._sub = this.myService.serviceReady$.subscribe({
           next: ready => {
              //Navigate to Overview or to specific machine if single station
              if (ready) {
                 //some  other code to redirect
                 //if (condition) this.router.navigate(['mainPage'])
                 //else this.router.navigate(['secondPage'])
                 observer.next(false);

                 observer.complete();

              }
           }
        }
     );
   });
  }
}
G.D.
  • 75
  • 9

2 Answers2

2

Saying the observable is provided in root so it is a singleton therefore I don't need to unsubscribe is completely the wrong way around. The fact that it is a singleton means this is where you really need to unsubscribe. If an observable falls to garbage collection then unsubscribing is not important, but a root provided service, this is where you cause memory leaks by not managing subscriptions.

This why my first advise to new Angular devs is always learn RxJs before you start to learn Angular. Subscribing to an observable and constructing a new observable is basically a map. The thing about mapping an observable to a new observable is you wont need to manage any subscriptions.

This really doesn't make much sense to me but it seems your route guard will wait for the service to be ready then based on another condition redirect to one page or another.

@Injectable({
   providedIn: 'root'
})

export class MyRedirectGuard implements CanActivate {
   constructor(private myService: MyService,
               private router: Router) {
   } 

canActivate(next: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable<boolean | UrlTree> {
    return this.myService.serviceReady$.pipe(
      filter(ready => ready), // filter out the service not being ready
      map(_ => { // We got through the filter so we know it's ready
        //some  other code to redirect
        //if (condition) this.router.navigate(['mainPage'])
        //else this.router.navigate(['secondPage'])
        return false;
      })
    );
  }
}

Again this doesn't make sense to me why you would use a route guard like this. You are waiting for a service to be ready then always denying the route and redirecting on a different condition. I would not allow a pull request that come through with this sort of thing. Seems like that routing logic belongs somewhere else.

Anyway, I hope my answer helps you understand that mapping one observable to another is easier than subscribing, constructing a new observable and then managing the subscription.

Adrian Brand
  • 20,384
  • 4
  • 39
  • 60
  • reason of this logic is too long to explain and maybe also a wrong approach to the problem. By the way like your solution. Rxjs is a huge topic to cover and requires time to improve skills in real implementation, but I'm learning I promise. In you example the final map operator is still returning an observable. Observable requires to be subscribed to emit, I imagine is angular job to subscribe and unsubscribe to this final observable. (this is my actual concern about guards) – G.D. Aug 22 '22 at 12:19
  • 1
    If your route guard returns an observable then it is the Angular framework that will do the subscribing. It will manage the subscription to the returned observable and as soon as it gets a true or a false it will unsubscribe. – Adrian Brand Aug 22 '22 at 12:24
  • perfect and also if I use my strange logic with other services since with the map operator I'm not generating other subscription will be for sure a better and cleaner approach that do not require unsubscription. I will test this, thank you – G.D. Aug 22 '22 at 12:27
  • fo sure after play a little bit with this code, also if question was well formed please upvote and help me to help others – G.D. Aug 22 '22 at 12:30
0

u can remove this._sub =

and add:

private _destroy$ = new Subject<boolean>();

/**/serviceReady$.pipe(takeUntil(this.destroy$))./**/

ngOnDestroy(): void {
  this.destroy$.next(true);
  this.destroy$.complite();
}
Никита Середа
  • 1,991
  • 1
  • 14
  • 34
  • Thanks, I already use this approach in other parts of the project, but didn't realize probably I can use it also here – G.D. Aug 22 '22 at 12:02
  • and still have a doubt I don't believe the ngOnDestroy is actually called, I'm using this approach in components, not in injected calass or services like this one, fix my sentence if is wrong – G.D. Aug 22 '22 at 12:23
  • ngOnDestroy don't work only if u close browser/tab. for this u can use Hostlistener window:onbeforeunload (i use this for unlock some records with web sockets and tell global service (all services extend from 1) to destroy, so all app works fine) – Никита Середа Aug 23 '22 at 07:06
  • I think when reload there are no problems at all, also if you leave subscription open. Complete application is reloaded, so I should not warry about it. My probelm was on long running to be sure no possible memory leaks will occurs. Maybe did not understood the comment – G.D. Aug 23 '22 at 11:50
  • yes, i was working with big app (solutios for partsshop & auto services) and we was having memory leaks with not destroyed compoents (bec they was having subscriptions). so global service and all other extend from global. global tells everythere this.destroy$ & this.service.destroy$ so all waroks fine after this fix – Никита Середа Aug 25 '22 at 09:56