From c9cc68b73c9b8163b5df545199bbc4582500f6b2 Mon Sep 17 00:00:00 2001 From: Gero Posmyk-Leinemann Date: Fri, 14 Jan 2022 11:49:24 +0000 Subject: [PATCH 1/3] [server] Bump ws version 7.5.5 -> 7.5.6 This fixes a bug related to closing websockets: https://github.com/websockets/ws/commit/ed2b803905289fc57616c3aef6b1690a3ca282b9 --- yarn.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/yarn.lock b/yarn.lock index 691084fce41231..acd61de7f0ead5 100644 --- a/yarn.lock +++ b/yarn.lock @@ -17599,9 +17599,9 @@ ws@^6.2.1: async-limiter "~1.0.0" ws@^7.4.6: - version "7.5.5" - resolved "https://registry.yarnpkg.com/ws/-/ws-7.5.5.tgz#8b4bc4af518cfabd0473ae4f99144287b33eb881" - integrity sha512-BAkMFcAzl8as1G/hArkxOxq3G7pjUqQ3gzYbLL0/5zNkph70e+lCoxBGnm6AW1+/aiNeV4fnKqZ8m4GZewmH2w== + version "7.5.6" + resolved "https://registry.yarnpkg.com/ws/-/ws-7.5.6.tgz#e59fc509fb15ddfb65487ee9765c5a51dec5fe7b" + integrity sha512-6GLgCqo2cy2A2rjCNFlxQS6ZljG/coZfZXclldI8FB/1G3CCI36Zd8xy2HrFVACi8tfk5XrgLQEk+P0Tnz9UcA== xcase@^2.0.1: version "2.0.1" From 73bc7a2618113e45a7408c4e0cc9f716378583f7 Mon Sep 17 00:00:00 2001 From: Gero Posmyk-Leinemann Date: Fri, 14 Jan 2022 15:49:38 +0000 Subject: [PATCH 2/3] [server] Avoid bubbling up any errors into ws.close(...) --- .../websocket/websocket-connection-manager.ts | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/components/server/src/websocket/websocket-connection-manager.ts b/components/server/src/websocket/websocket-connection-manager.ts index 1baefea131c97c..e2acd595e763f2 100644 --- a/components/server/src/websocket/websocket-connection-manager.ts +++ b/components/server/src/websocket/websocket-connection-manager.ts @@ -202,14 +202,19 @@ export class WebsocketConnectionManager implements ConnectionHandler { gitpodServer.initialize(client, user, resourceGuard, clientContext.clientMetadata, connectionCtx, clientHeaderFields); client.onDidCloseConnection(() => { - gitpodServer.dispose(); - increaseApiConnectionClosedCounter(); - this.events.emit(EVENT_CONNECTION_CLOSED, gitpodServer, expressReq); - - clientContext.removeEndpoint(gitpodServer); - if (clientContext.hasNoEndpointsLeft()) { - this.contexts.delete(clientContext.clientId); - this.events.emit(EVENT_CLIENT_CONTEXT_CLOSED, clientContext); + try { + gitpodServer.dispose(); + increaseApiConnectionClosedCounter(); + this.events.emit(EVENT_CONNECTION_CLOSED, gitpodServer, expressReq); + + clientContext.removeEndpoint(gitpodServer); + if (clientContext.hasNoEndpointsLeft()) { + this.contexts.delete(clientContext.clientId); + this.events.emit(EVENT_CLIENT_CONTEXT_CLOSED, clientContext); + } + } catch (err) { + // we want to be absolutely sure that we do not bubble up errors into ws.onClose here + log.error("onDidCloseConnection", err); } }); clientContext.addEndpoint(gitpodServer); From b6025ef79d1e0cbdc4d840fae9732d2ace54f34a Mon Sep 17 00:00:00 2001 From: Gero Posmyk-Leinemann Date: Fri, 14 Jan 2022 15:50:33 +0000 Subject: [PATCH 3/3] [server] Add experimental ws closing --- .../server/src/express/ws-ping-pong-handler.ts | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/components/server/src/express/ws-ping-pong-handler.ts b/components/server/src/express/ws-ping-pong-handler.ts index b44f06dae848c9..7b3cfccf6a7033 100644 --- a/components/server/src/express/ws-ping-pong-handler.ts +++ b/components/server/src/express/ws-ping-pong-handler.ts @@ -30,9 +30,21 @@ export class WsPingPongHandler implements Disposable { try { switch (ws.readyState) { case websocket.CLOSED: - // ws should not be in the clients list anymore + // 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 }); - return + + // 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); + } + } + return; case websocket.CONNECTING: // ws should not be in the clients list, yet log.warn("websocket in strange state", { readyState: ws.readyState });