[server] make sure to release websocket clients #10193
Conversation
| /werft run 👍 started the job as gitpod-build-at-release-ws.2 |
AlexTugarev force-pushed the at/release-ws branch from eb2b5c7 to 957ef79 Compare 
| this.clients.delete(ws); | ||
| emitClose(); | ||
| log.warn( | ||
| "websocket in strange state. forcefully emitting a close event to release resources.", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
AlexTugarev force-pushed the at/release-ws branch from 957ef79 to 381d35c Compare 
roboquat added deployed: webapp 
What was initially introduces as some experimental try, seems to be a reasonable way to mitigate server leaks.
The way websocket connections are bounds to jsonrpc servers, which themselves integrate with the rest of the system, makes it necessary to have a circuit breaker in place to guarantee that stale client connections do not cause leakage of resource.
How to test
Unfortunately, it's hard to provoke the otherwise missing
closeevent.