Skip to content

Commit 381d35c

Browse files
committed
[server] make sure to release websocket clients
1 parent acc55c4 commit 381d35c

File tree

1 file changed

+17
-13
lines changed

1 file changed

+17
-13
lines changed

components/server/src/express/ws-connection-handler.ts

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { Disposable, DisposableCollection } from "@gitpod/gitpod-protocol";
1010
import { repeat } from "@gitpod/gitpod-protocol/lib/util/repeat";
1111
import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
1212
import { WsNextFunction, WsRequestHandler } from "./ws-handler";
13+
import { ClientMetadata } from "../websocket/websocket-connection-manager";
1314

1415
/**
1516
* This class provides a websocket handler that manages ping-pong behavior for all incoming websocket requests.
@@ -30,19 +31,20 @@ export class WsConnectionHandler implements Disposable {
3031
try {
3132
switch (ws.readyState) {
3233
case websocket.CLOSED:
33-
// ws should not be in the clients list anymore, but still happens:
34-
// we rely on a 'close' event being generated, but never receive it. At the same time, the readyState is 'CLOSED' (3).
35-
// judging from the ws source code, this might only happen if an earlier registered handler throws an (unhandled) error.
36-
log.warn("websocket in strange state", { readyState: ws.readyState });
37-
38-
// the following is a hack trying to mitigate the effects of leaking CLOSED websockets
39-
if (process.env.EXPERIMENTAL_WS_TERMINATION) {
40-
try {
41-
(ws as any).emitClose();
42-
log.warn("websocket (experimental): close emitted");
43-
} catch (err) {
44-
log.error("websocket (experimental): error on emit('close')", err);
45-
}
34+
// (AT) for unknown reasons the expected `close` event was not emitted, otherwise this websocket would
35+
// no longer be contained in the list of clients iterated over. make sure we release all resources bound
36+
// to this client connection.
37+
const emitClose = (ws as any).emitClose;
38+
if (typeof emitClose === "function") {
39+
this.clients.delete(ws);
40+
41+
emitClose();
42+
log.warn(
43+
"websocket in strange state. forcefully emitting a close event to release resources.",
44+
{
45+
clientMetadata: (ws as any).clientMetadata,
46+
},
47+
);
4648
}
4749
return;
4850
case websocket.CONNECTING:
@@ -89,6 +91,8 @@ export class WsConnectionHandler implements Disposable {
8991

9092
handler(): WsRequestHandler {
9193
return (ws: websocket, req: express.Request, next: WsNextFunction) => {
94+
// attaching ClientMetadata to websocket to use it for logging on websocket errors
95+
(ws as any).clientMetadata = ClientMetadata.fromRequest(req);
9296
// maintain set of clients
9397
this.clients.add(ws);
9498
ws.on("close", () => this.clients.delete(ws));

0 commit comments

Comments
 (0)