kunwp1 commented on code in PR #8855:
URL: https://github.com/apache/texera/pull/8855#discussion_r4202340772
##########
frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.spec.ts:
##########
@@ -283,6 +283,155 @@ describe("WorkflowWebsocketService", () => {
}
}));
+ /** A frame from the server, which is what marks the connection up. */
+ const serverSends = (socket: FakeWebSocket) =>
+ socket.onmessage?.(
+ new MessageEvent("message", { data: JSON.stringify({ type:
"WorkflowStateEvent", state: "RUNNING" }) })
+ );
+
+ /** A server-side close frame as the browser reports it: already CLOSED,
wasClean true. */
+ const serverCloses = (socket: FakeWebSocket, code: number) => {
+ socket.readyState = FakeWebSocket.CLOSED;
+ socket.onclose?.(new CloseEvent("close", { wasClean: true, code, reason:
"" }));
+ };
+
+ /** Clean-close scenario shared by the per-close-code tests below; runs
inside the caller's fakeAsync zone. */
+ const expectCleanCloseRedials = (code: number) => {
+ const statuses: boolean[] = [];
+ const statusSubscription =
service.getConnectionStatusStream().subscribe(value => statuses.push(value));
+
+ try {
+ service.openWebsocket(1, 1, 1);
+ flushMicrotasks();
+ serverSends(openedSockets[0]);
+ expect(statuses).toEqual([false, true]);
+
+ serverCloses(openedSockets[0], code);
+
+ expect(statuses).toEqual([false, true, false]);
+ expect(service.isConnected).toBe(false);
+
+ tick(WS_RECONNECT_INTERVAL_MS - 1);
+ expect(openedSockets.length).toBe(1);
+ tick(1);
+ flushMicrotasks();
+ expect(openedSockets.length).toBe(2);
+ } finally {
+ statusSubscription.unsubscribe();
+ service.closeWebsocket();
+ }
+ };
+
+ // fakeAsync bodies cannot be wrapped by it.each (no ProxyZone), so one it
per close code.
+ it("a clean server-side close reports the drop and redials after
WS_RECONNECT_INTERVAL_MS (code 1000)", fakeAsync(() =>
+ expectCleanCloseRedials(1000)));
+ it("a clean server-side close reports the drop and redials after
WS_RECONNECT_INTERVAL_MS (code 1001)", fakeAsync(() =>
+ expectCleanCloseRedials(1001)));
+ it("a clean server-side close reports the drop and redials after
WS_RECONNECT_INTERVAL_MS (code 1005)", fakeAsync(() =>
+ expectCleanCloseRedials(1005)));
+ it("a clean server-side close reports the drop and redials after
WS_RECONNECT_INTERVAL_MS (code 1012)", fakeAsync(() =>
+ expectCleanCloseRedials(1012)));
+
+ it("closeWebsocket() does not redial", fakeAsync(() => {
+ service.openWebsocket(1, 1, 1);
+ flushMicrotasks();
+
+ service.closeWebsocket();
+ tick(WS_RECONNECT_INTERVAL_MS * 5);
+ flushMicrotasks();
+
+ expect(openedSockets.length).toBe(1);
+ expect(openedSockets[0].readyState).toBe(FakeWebSocket.CLOSED);
+ }));
+
+ it("redials after each of two consecutive clean server-side closes",
fakeAsync(() => {
+ try {
+ service.openWebsocket(1, 1, 1);
+ flushMicrotasks();
+ serverSends(openedSockets[0]);
+ expect(service.isConnected).toBe(true);
+
+ serverCloses(openedSockets[0], 1001);
+ expect(service.isConnected).toBe(false);
+ tick(WS_RECONNECT_INTERVAL_MS);
+ flushMicrotasks();
+ expect(openedSockets.length).toBe(2);
+
+ serverSends(openedSockets[1]);
+ expect(service.isConnected).toBe(true);
+
+ // The redialed chain has to carry the same completion handling as the
first dial.
+ serverCloses(openedSockets[1], 1001);
+ expect(service.isConnected).toBe(false);
+ tick(WS_RECONNECT_INTERVAL_MS);
+ flushMicrotasks();
+ expect(openedSockets.length).toBe(3);
+ } finally {
+ service.closeWebsocket();
+ }
+ }));
+
+ it("re-opening during the reconnect gap cancels the pending redial",
fakeAsync(() => {
+ try {
+ service.openWebsocket(1, 1, 1);
+ flushMicrotasks();
+ serverCloses(openedSockets[0], 1001);
+ tick(1000);
Review Comment:
Maybe utilize WS_RECONNECT_INTERVAL_MS instead of hard-coded magic numbers
1000 and 5000.
##########
frontend/src/app/workspace/service/workflow-websocket/workflow-websocket.service.ts:
##########
@@ -117,6 +117,9 @@ export class WorkflowWebsocketService {
this.websocket = webSocket<TexeraWebsocketEvent |
TexeraWebsocketRequest>(websocketUrl);
// setup reconnection logic
const wsWithReconnect = this.websocket.pipe(
+ // A server-side close with a close frame completes the socket instead
of erroring;
+ // turn the completion into an error so the retry below reports the drop
and redials.
+ concatWith(throwError(() => new Error("websocket closed by the
server"))),
Review Comment:
Take the admin execution page where an admin clicks Kill on workflow 42. The
server is saying "I won't accept this", and the page keeps redialing every 3
seconds until the tab is closed. And each extra Kill click starts another loop.
Could we add a limit on this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
