Skip to content

[server] make sure to release websocket clients #10193

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
roboquat merged 1 commit into from
May 24, 2022
Merged

[server] make sure to release websocket clients #10193

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions components/server/src/express/ws-connection-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { Disposable, DisposableCollection } from "@gitpod/gitpod-protocol";
import { repeat } from "@gitpod/gitpod-protocol/lib/util/repeat";
import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
import { WsNextFunction, WsRequestHandler } from "./ws-handler";
import { ClientMetadata } from "../websocket/websocket-connection-manager";

/**
* This class provides a websocket handler that manages ping-pong behavior for all incoming websocket requests.
Expand All @@ -30,19 +31,20 @@ export class WsConnectionHandler implements Disposable {
try {
switch (ws.readyState) {
case websocket.CLOSED:
// ws should not be in the clients list anymore, but still happens:
// we rely on a 'close' event being generated, but never receive it. At the same time, the readyState is 'CLOSED' (3).
// judging from the ws source code, this might only happen if an earlier registered handler throws an (unhandled) error.
log.warn("websocket in strange state", { readyState: ws.readyState });

// the following is a hack trying to mitigate the effects of leaking CLOSED websockets
if (process.env.EXPERIMENTAL_WS_TERMINATION) {
try {
(ws as any).emitClose();
log.warn("websocket (experimental): close emitted");
} catch (err) {
log.error("websocket (experimental): error on emit('close')", err);
}
// (AT) for unknown reasons the expected `close` event was not emitted, otherwise this websocket would
// no longer be contained in the list of clients iterated over. make sure we release all resources bound
// to this client connection.
const emitClose = (ws as any).emitClose;
if (typeof emitClose === "function") {
this.clients.delete(ws);

emitClose();
log.warn(
"websocket in strange state. forcefully emitting a close event to release resources.",
Copy link
Member

@easyCZ easyCZ May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A metric would be better here than a log. Alternatively, enrich it with some data - ie Connection ID to make it useful for tracking down which connection.
Copy link
Member Author

@AlexTugarev AlexTugarev May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading on https://github.com/websockets/ws/blob/master/lib/websocket.js#L245-L259, we shouldn't even end up in this situation.

What kind of metric would you suggest? Assuming this is a bug, we want to log it. +1 for logging more details on the connection. Not sure which id is meant, I'll go with full ClientMetadata.
Copy link
Member Author

@AlexTugarev AlexTugarev May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done
Copy link
Member

@easyCZ easyCZ May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically it comes down to what is it you're trying to observe in the system. Presumably you're logging it so that you can get an idea that it is happening. For that use-case, a counter metric you add to a dashboard is a better choice - it's cheaper and doesn't require deep context.

If, however, this is just cleanup and we don't really care about how often, or details, of the problem, we might as well not log at all.
{
clientMetadata: (ws as any).clientMetadata,
},
);
}
return;
case websocket.CONNECTING:
Expand Down Expand Up @@ -89,6 +91,8 @@ export class WsConnectionHandler implements Disposable {

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