Skip to content

Commit c81f494

Browse files
dsteelma-umdPerplexity AI
andcommitted
LIBDRUM-1005. Refactored pipeline avoid possible memory link
Refactored the `this.bitstream$.pipe` pipeline based suggestions from Perplexity AI that the existing code had: 1) A memory leak if the pipeline doesn't complete, in Perplexity's explanation: > When Angular destroys this component (e.g., the user navigates away), > the outer  .subscribe()  is not automatically cancelled. If the > observable chain hasn’t completed yet — for example, the `switchMap` > is still waiting for `isAuthorized$` or `isLoggedIn$` to emit — the > subscription lives on in memory, holding a reference to the destroyed > component and all its injected services. This prevents the garbage > collector from reclaiming that memory. > > In practice, the  take(1)  operator in the pipe mitigates this > somewhat, because once a single value passes through, `take(1)` > completes the stream and the subscription auto-unsubscribes. However, > if the upstream observables (`isAuthorized$`, `isLoggedIn$`) never > emit (due to an error or unexpected delay), the subscription hangs > indefinitely. The `take(1)` only protects you in the happy path. This seems like a reasonable refactoring, because it is possible that the authorization/authentication calls to the backend could fail. 2) Nested subscriptions are a rxjs anti-pattern. See https://medium.com/ngconf/why-you-shouldnt-nest-subscribes-eafbc3b00af2 When pushed, Perplexity conceded that the nested subscription is not likely a problem (more of a "code smell") in this case, because the calls to the TranslateService in the inner subscription complete synchronously, and so shouldn't cause a problem. Seemed like a reasonable idea, so implemented it. Co-authored-by: Perplexity AI <noreply@perplexity.ai> https://umd-dit.atlassian.net/browse/LIBDRUM-1005
1 parent 555fe32 commit c81f494

1 file changed

Lines changed: 22 additions & 13 deletions

File tree

src/app/restricted-access/restricted-access.component.ts

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,11 @@ import {
55
} from '@angular/common';
66
import {
77
Component,
8+
DestroyRef,
9+
inject,
810
OnInit,
911
} from '@angular/core';
12+
import { takeUntilDestroyed } from '@angular/core/rxjs-interop';
1013
import {
1114
ActivatedRoute,
1215
Router,
@@ -18,6 +21,7 @@ import {
1821
import {
1922
BehaviorSubject,
2023
combineLatest as observableCombineLatest,
24+
EMPTY,
2125
filter,
2226
map,
2327
Observable,
@@ -68,6 +72,8 @@ export class RestrictedAccessComponent implements OnInit {
6872
bitstreamRD$: Observable<RemoteData<Bitstream>>;
6973
bitstream$: Observable<Bitstream>;
7074

75+
private destroyRef = inject(DestroyRef);
76+
7177
constructor(
7278
private route: ActivatedRoute,
7379
protected router: Router,
@@ -112,15 +118,17 @@ export class RestrictedAccessComponent implements OnInit {
112118
return [isAuthorized, isLoggedIn, bitstream, fileLink];
113119
}));
114120
} else {
115-
return [[isAuthorized, isLoggedIn, bitstream, '']];
121+
return observableOf([isAuthorized, isLoggedIn, bitstream, ''] as [boolean, boolean, Bitstream, string]);
116122
}
117123
}),
118-
).subscribe(([isAuthorized, isLoggedIn, bitstream, fileLink]: [boolean, boolean, Bitstream, string]) => {
119-
if (isAuthorized && isNotEmpty(fileLink)) {
120-
// This shouldn't happen, as the download is authorized, and the file link is available, so just redirect to
121-
// actual download page.
122-
this.hardRedirectService.redirect(fileLink);
123-
} else {
124+
switchMap(([isAuthorized, isLoggedIn, bitstream, fileLink]: [boolean, boolean, Bitstream, string]) => {
125+
if (isAuthorized && isNotEmpty(fileLink)) {
126+
// This shouldn't happen, as the download is authorized, and the file link is available, so just redirect to
127+
// actual download page.
128+
this.hardRedirectService.redirect(fileLink);
129+
return EMPTY;
130+
}
131+
124132
let header$: Observable<string>;
125133
let message$: Observable<string>;
126134

@@ -130,7 +138,7 @@ export class RestrictedAccessComponent implements OnInit {
130138
this.responseService.setForbidden();
131139
header$ = this.translateService.get('bitstream.restricted-access.user.forbidden.header', {});
132140

133-
if (bitstream && bitstream.metadata['dc.title'] && bitstream.metadata['dc.title'][0] && bitstream.metadata['dc.title'][0].value) {
141+
if (bitstream && bitstream.metadata['dc.title'] && bitstream.metadata['dc.title'][0] && bitstream.metadata['dc.title'][0].value) {
134142
const filename = bitstream.metadata['dc.title'][0].value;
135143
message$ = this.translateService.get(
136144
'bitstream.restricted-access.user.forbidden.with_file.message', { 'filename': filename });
@@ -145,11 +153,12 @@ export class RestrictedAccessComponent implements OnInit {
145153
[header$, message$] = this.configureAnonymous(bitstream);
146154
}
147155

148-
zip(header$, message$).subscribe(([header, message]) => {
149-
this.restrictedAccessHeader.next(header);
150-
this.restrictedAccessMessage.next(message);
151-
});
152-
}
156+
return zip(header$, message$);
157+
}),
158+
takeUntilDestroyed(this.destroyRef),
159+
).subscribe(([header, message]: [string, string]) => {
160+
this.restrictedAccessHeader.next(header);
161+
this.restrictedAccessMessage.next(message);
153162
});
154163
}
155164

0 commit comments

Comments
 (0)