[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.