Should I unsubscribe Observables lists?

@Tommertom, So that include assigning undefined to the subject?

1 Like

So to summarise more complete: streams that do not complete during the lifetime of component need unsubscription, which ideally should be done by forcing the completion using the patterns shown above (or use async pipe). And if you reference to your subscription using a variable, assign it to undefined after completion.

(thx @jaydz)

1 Like

Got it. Thanks to you too @Tommertom

He wasn’t undefining the subscription, but the tripwire Subject. To my eye, that’s unnecessary (esp in this toy example) but good style, because if the Subject emits unexpectedly, it could have consequences for everything still listening to it. If you undefine it as part of your teardown logic,you guarantee yourself that a source of hard-to-debug errors will not exist.

All that said, I’d already seen the article @rapropos linked, and I wasn’t convinced by it. I think there’s a benefit to forcing yourself to know exactly what you’re tearing down in your teardown logic. But it’s a very clever idea. Maybe if you have 50 Observables on your page, and you need to edit some in and out in each update? But I only have a few Observables per “code piece.”: (Though I have a couple dozen providers in one project, so I just segmented things that way.)

@AaronSterling, It seems strange that a Subject could emit after completion. Chalk it up to the imperfections of computer tech??

I certainly don’t have 50 observables on any page, but I do tend to have 2 - 3 observables on init that get items like a user id, an app state boolean or string, etc.

Those I always use take(1) on so don’t need to think twice about them. But with the addition of a couple more Observables, I guess I just prefer reducing my code by a few lines when possible. Or I just like feeling clever.

Though avoiding importing { Subscription }, Typing each subscription, then assigning and unsubscribing does seem to reduce my lines to a decent degree.

I am curious about

that line though.

I like having teardown logic live right next to buildup logic. It’s sort of a cousin to why I prefer using initializers over splitting up declarations and doing initialization in constructors (where possible). Then if I add a new Observable later, I only have to edit one place, because it can share the same tripwire. Golang has this concept burned into the language itself with its defer statement, and I find it very helpful in dealing with things like rolling back SQL transactions in error situations.

1 Like

I second that notion.

Looks like I’m on team @rapropos on this one.

Do you mean:

thing = {
  buildUp: doThis(),
  tearDown: doThat(),
  whileAlive: doMainTask()
}

because I can see the benefit of that. I now wonder if I could extend Observable to that, using ideas from this thread? That might be super clean.

If you screwed up your code and the Subject did something you didn’t intend for it to do. Finding that error can be a pain and a half, because your mistake might be removed from the effect you are seeing.

Sorry to interject, but this is how I am implementing the “tripwire effect”

  this.detailService.filterSolutions().pipe(takeUntil(this.destroy$))
  .subscribe(data => { 
   if (data) { this.solutions = data; }
   });

Not sure if it’s applicable to the go-between with you and @rapropos
I’m fetching a switchMap / query action from detailService, and implementing it in my constructor.
My provider of course contains some logic, i. e

searchSolution(item: Problem | null) {
   this._solutionTerms$.next(item);
 }

filterSolutions() : Observable<Solution[]>{
   return this._solutionTerms$.pipe(switchMap(item => getSolutions(item)));
 }

The syntax you suggest seems reasonable. I’m less tied to how it’s written than where, because I generally find bugs caused by interactions of diffusely written code to be simultaneously easy to make and hard to diagnose. This is partially why I love GC and DI so much - manually managing object lifecycle has caused me no end of frustration over the years.

Makes sense. I’ll add that extra layer of defense to my Subjects.

I recently encountered a particularly nasty bug related to this concept.

export class AuthService {
  authtoken = new ReplaySubject<string>(1);
  // imagine other things calling `next` to set authorization tokens
}

export class AuthHttpInterceptor implements HttpInterceptor {
  constructor(private _auth: AuthService) {
  }

  intercept(req: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>> {
    return this._auth.authtoken
      .pipe(
        mergeMap((jwt) => {
        let areq = req.clone({setHeaders: {Authorization: 'Bearer ' + jwt}});
        return next.handle(areq);
      }));
  }
}

export class OtherService {
  constructor(private _http: HttpClient) {}

  thingy(): Observable<Thingy> {
    return forkJoin(this._http.get('/thing1'), this._http.get('/thing2').pipe(
      mergeMap(things => frobulate(things))
    );
  }
}

export class Page {
  thingy: Thingy;

  constructor(private _svc: OtherService) {}

  fetchThingy(): void {
    this._svc.thingy().subscribe(thingy => this.thingy = thingy);
  }
}

This is also related to the “diffuse coding bug” topic, because it’s the interceptor that unwittingly causes the bug. OtherService.thingy() will never emit.

After several hours of head-scratching and narrowing, I realized that since the authtoken ReplaySubject never completes, neither will any of the intercepted HTTP requests (although they will emit, which means I didn’t notice the interceptor bug when just simply subscribing to requests, only when attempting to forkJoin multiple ones).

A solution is to add first() to the beginning of the interceptor’s pipe.

Why first() as opposed to take(1) ?
To emit an error if necessary?

take(1) isn’t good enough. My favorite example of this problem is

obs.filter(x => x!==null).take(1)

obs never completes if it never emits a non-null value, despite the presence of take(1). So it’s potentially a long term memory leak that looks super safe.

Pretty much. Imagine we have a “continue as guest” option that gives up on being authorized, so the token stream could complete without ever having sent anything. I think I would prefer any pending authorization-needed intercepted requests that are waiting on tokens to recognize that situation as “not normal”.

Definitely good to know. Was not aware of that.

The app i’m working on right now just so happens to have a isVisitor status that I am observing, so that’s a very helpful point for me.

Thanks @rapropos

For anybody who stumbles across this conversation later, I have recently become aware of ngx-take-until-destroy, which automates part of this tripwire idiom.

2 Likes

In my most recent apps I use Ngrx and manage to do without a manual subsribe, just using async with ngIf and ngFor. If there is a neeed to fill a field for the component (specific Ui state like a form) I use tap on the pipe.

So all in the angular template with local variables. And as such sometimes async one observable stream twice in the template (eg. title and content using ngIf)

Saves quite some lines and I assume more performing then doing all stuff manually…

Any considerations?