bito-code-review[bot] commented on PR #40856:
URL: https://github.com/apache/superset/pull/40856#issuecomment-4651343221
<!-- Bito Reply -->
The architectural review comment is correct. The current implementation of
`connectionLimitReason` relies on `Object.keys(sockets).length` and
`channels[channel]?.sockets.length`, which include stale entries that have not
yet been garbage collected. This can lead to premature connection rejection.
To resolve this, you should filter the registries to count only active
sockets before enforcing the limits. You can achieve this by adding a helper to
filter out closed sockets or by invoking the existing cleanup logic before
checking the counts.
Here is a minimal, localized fix for `superset-websocket/src/index.ts`:
```typescript
export const connectionLimitReason = (channel: string): string | null => {
const { maxTotalConnections, maxConnectionsPerChannel } = opts;
// Filter out closed sockets to get an accurate count
const activeSockets = Object.values(sockets).filter(s => s.ws.readyState
=== WebSocket.OPEN);
if (
maxTotalConnections > 0 &&
activeSockets.length >= maxTotalConnections
) {
return `total connection limit (${maxTotalConnections}) reached`;
}
if (maxConnectionsPerChannel > 0) {
const channelSockets = channels[channel]?.sockets.filter(s =>
s.ws.readyState === WebSocket.OPEN) ?? [];
if (channelSockets.length >= maxConnectionsPerChannel) {
return `per-channel connection limit (${maxConnectionsPerChannel})
reached`;
}
}
return null;
};
```
Would you like me to check the other comments on this PR and implement fixes
for them as well?
**superset-websocket/src/index.ts**
```
export const connectionLimitReason = (channel: string): string | null => {
const { maxTotalConnections, maxConnectionsPerChannel } = opts;
// Filter out closed sockets to get an accurate count
const activeSockets = Object.values(sockets).filter(s => s.ws.readyState
=== WebSocket.OPEN);
if (
maxTotalConnections > 0 &&
activeSockets.length >= maxTotalConnections
) {
return `total connection limit (${maxTotalConnections}) reached`;
}
if (maxConnectionsPerChannel > 0) {
const channelSockets = channels[channel]?.sockets.filter(s =>
s.ws.readyState === WebSocket.OPEN) ?? [];
if (channelSockets.length >= maxConnectionsPerChannel) {
return `per-channel connection limit (${maxConnectionsPerChannel})
reached`;
}
}
return null;
};
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
