Skip to content

Commit b3db209

Browse files
fix(frontend): unsubscribe connection-status subscription in WorkflowWebsocketService.closeWebsocket (#4377)
### What changes were proposed in this PR? `WorkflowWebsocketService.openWebsocket` subscribed to the internal `webSocketResponseSubject` observable to track cluster status and mark the connection as live, but did not store the returned `Subscription`. As a result, `closeWebsocket` could not unsubscribe it, causing the handler to accumulate on every call to `openWebsocket` (e.g. when the user switches workflows or computing units). **Root cause (before):** ```typescript // openWebsocket() — subscription discarded, never cleaned up this.websocketEvent().subscribe(evt => { ... }); // closeWebsocket() — only wsWithReconnectSubscription was torn down this.wsWithReconnectSubscription?.unsubscribe(); ``` **Fix (after):** ```typescript // openWebsocket() this.statusUpdateSubscription = this.websocketEvent().subscribe(evt => { ... }); // closeWebsocket() this.wsWithReconnectSubscription?.unsubscribe(); this.statusUpdateSubscription?.unsubscribe(); // ← new ``` The new `statusUpdateSubscription` field is declared as `Subscription | undefined`; the `?.unsubscribe()` call is safely a no-op when `closeWebsocket` is called before `openWebsocket` was ever invoked. ### Any related issues, documentation, discussions? Closes #4376 ### How was this PR tested? Two new unit tests were added to `workflow-websocket.service.spec.ts`: "should close the previous status subscription when openWebsocket is called again" uses a lightweight WebSocket test double, calls openWebsocket() twice and closeWebsocket() once, and verifies that the previous statusUpdateSubscription is torn down on each reopen and on close The existing service-creation test continues to pass. ### Was this PR authored or co-authored using generative AI tooling? No. --------- Signed-off-by: Asish Kumar <officialasishkumar@gmail.com> Co-authored-by: Meng Wang <125719918+mengw15@users.noreply.github.com>
1 parent d547c76 commit b3db209

2 files changed

Lines changed: 63 additions & 1 deletion

File tree

frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.spec.ts

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,44 @@ import { TestBed } from "@angular/core/testing";
2121
import { WorkflowWebsocketService } from "./workflow-websocket.service";
2222
import { commonTestProviders } from "../../../common/testing/test-utils";
2323

24+
/** Browser-like WebSocket test double used to verify websocket reopen and subscription cleanup behavior. */
25+
class FakeWebSocket extends EventTarget {
26+
public static readonly CONNECTING = 0;
27+
public static readonly OPEN = 1;
28+
public static readonly CLOSING = 2;
29+
public static readonly CLOSED = 3;
30+
31+
public readyState = FakeWebSocket.CONNECTING;
32+
33+
constructor(public readonly url: string) {
34+
super();
35+
Promise.resolve().then(() => {
36+
this.readyState = FakeWebSocket.OPEN;
37+
const onopen = this.onopen;
38+
onopen?.(new Event("open"));
39+
this.dispatchEvent(new Event("open"));
40+
});
41+
}
42+
43+
public onopen: ((ev: Event) => unknown) | null = null;
44+
public onclose: ((ev: CloseEvent) => unknown) | null = null;
45+
public onerror: ((ev: Event) => unknown) | null = null;
46+
public onmessage: ((ev: MessageEvent) => unknown) | null = null;
47+
48+
public send() {}
49+
50+
public close() {
51+
if (this.readyState === FakeWebSocket.CLOSED) {
52+
return;
53+
}
54+
this.readyState = FakeWebSocket.CLOSED;
55+
const closeEvent = new CloseEvent("close", { wasClean: true, code: 1000, reason: "" });
56+
const onclose = this.onclose;
57+
onclose?.(closeEvent);
58+
this.dispatchEvent(closeEvent);
59+
}
60+
}
61+
2462
describe("WorkflowWebsocketService", () => {
2563
let service: WorkflowWebsocketService;
2664

@@ -34,4 +72,26 @@ describe("WorkflowWebsocketService", () => {
3472
it("should be created", () => {
3573
expect(service).toBeTruthy();
3674
});
75+
76+
it("should close the previous status subscription when openWebsocket is called again", () => {
77+
const originalWebSocket = window.WebSocket;
78+
window.WebSocket = FakeWebSocket as unknown as typeof WebSocket;
79+
80+
try {
81+
service.openWebsocket(1, 1, 1);
82+
const firstStatusSubscription = (service as any).statusUpdateSubscription;
83+
expect(firstStatusSubscription.closed).toBeFalse();
84+
85+
service.openWebsocket(1, 1, 1);
86+
expect(firstStatusSubscription.closed).toBeTrue();
87+
88+
const secondStatusSubscription = (service as any).statusUpdateSubscription;
89+
expect(secondStatusSubscription.closed).toBeFalse();
90+
91+
service.closeWebsocket();
92+
expect(secondStatusSubscription.closed).toBeTrue();
93+
} finally {
94+
window.WebSocket = originalWebSocket;
95+
}
96+
});
3797
});

frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ export class WorkflowWebsocketService {
4747

4848
private websocket?: WebSocketSubject<TexeraWebsocketEvent | TexeraWebsocketRequest>;
4949
private wsWithReconnectSubscription?: Subscription;
50+
private statusUpdateSubscription?: Subscription;
5051
private readonly webSocketResponseSubject: Subject<TexeraWebsocketEvent> = new Subject();
5152
private readonly connectionStatusSubject = new BehaviorSubject<boolean>(false);
5253

@@ -90,6 +91,7 @@ export class WorkflowWebsocketService {
9091

9192
public closeWebsocket() {
9293
this.wsWithReconnectSubscription?.unsubscribe();
94+
this.statusUpdateSubscription?.unsubscribe();
9395
this.websocket?.complete();
9496
this.updateConnectionStatus(false);
9597
}
@@ -132,7 +134,7 @@ export class WorkflowWebsocketService {
132134
);
133135

134136
// refresh connection status
135-
this.websocketEvent().subscribe(evt => {
137+
this.statusUpdateSubscription = this.websocketEvent().subscribe(evt => {
136138
if (evt.type === "ClusterStatusUpdateEvent") {
137139
this.numWorkers = evt.numWorkers;
138140
}

0 commit comments

Comments
 (0)