bito-code-review[bot] commented on code in PR #40856:
URL: https://github.com/apache/superset/pull/40856#discussion_r3377389402
##########
superset-websocket/spec/index.test.ts:
##########
@@ -442,6 +442,169 @@ describe('server', () => {
});
});
+ describe('connection limits', () => {
+ const getRequest = (token: string, url: string): http.IncomingMessage => {
+ const request = new http.IncomingMessage(new net.Socket());
+ request.method = 'GET';
+ request.headers = { cookie: `${config.jwtCookieName}=${token}` };
+ request.url = url;
+ return request;
+ };
+
+ afterEach(() => {
+ // restore opt-in limits to their disabled default
+ server.opts.maxTotalConnections = 0;
+ server.opts.maxConnectionsPerChannel = 0;
+ });
+
+ test('no limit when disabled (0)', () => {
+ server.opts.maxTotalConnections = 0;
+ server.opts.maxConnectionsPerChannel = 0;
+ const socketInstance = {
+ ws: new wsMock('localhost'),
+ channel: channelId,
+ pongTs: Date.now(),
+ };
+ server.trackClient(channelId, socketInstance);
+ expect(server.connectionLimitReason(channelId)).toBeNull();
+ });
+
+ test('total connection limit reached', () => {
+ server.opts.maxTotalConnections = 1;
+ const ws = new wsMock('localhost');
+ setReadyState(ws, WebSocket.OPEN);
+ const socketInstance = {
+ ws,
+ channel: channelId,
+ pongTs: Date.now(),
+ };
+ server.trackClient(channelId, socketInstance);
+ expect(server.connectionLimitReason('some-other-channel')).toMatch(
+ /total connection limit/,
+ );
+ });
+
+ test('per-channel connection limit reached', () => {
+ server.opts.maxConnectionsPerChannel = 1;
+ const ws = new wsMock('localhost');
+ setReadyState(ws, WebSocket.OPEN);
+ const socketInstance = {
+ ws,
+ channel: channelId,
+ pongTs: Date.now(),
+ };
+ server.trackClient(channelId, socketInstance);
+ expect(server.connectionLimitReason(channelId)).toMatch(
+ /per-channel connection limit/,
+ );
+ });
+
+ test('stale closed socket does not count toward total limit', () => {
+ server.opts.maxTotalConnections = 1;
+ const ws = new wsMock('localhost');
+ const socketInstance = {
+ ws,
+ channel: channelId,
+ pongTs: Date.now(),
+ };
+ server.trackClient(channelId, socketInstance);
+ // simulate the socket having closed but not yet been GC'd
+ setReadyState(ws, WebSocket.CLOSED);
+ expect(server.connectionLimitReason('some-other-channel')).toBeNull();
+ });
+
+ test('stale closed socket does not count toward per-channel limit', () => {
+ server.opts.maxConnectionsPerChannel = 1;
+ const ws = new wsMock('localhost');
+ const socketInstance = {
+ ws,
+ channel: channelId,
+ pongTs: Date.now(),
+ };
+ server.trackClient(channelId, socketInstance);
+ // simulate the socket having closed but not yet been GC'd
+ setReadyState(ws, WebSocket.CLOSED);
+ expect(server.connectionLimitReason(channelId)).toBeNull();
+ });
+
+ test('isSocketActive reflects the socket readyState', () => {
+ const ws = new wsMock('localhost');
+ setReadyState(ws, WebSocket.OPEN);
+ const socketId = server.trackClient(channelId, {
+ ws,
+ channel: channelId,
+ pongTs: Date.now(),
+ });
+ expect(server.isSocketActive(socketId)).toBe(true);
+ setReadyState(ws, WebSocket.CLOSED);
+ expect(server.isSocketActive(socketId)).toBe(false);
+ // unknown socket ids are never active
+ expect(server.isSocketActive('does-not-exist')).toBe(false);
+ });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete CONNECTING state test</b></div>
<div id="fix">
The `isSocketActive` test only covers OPEN and CLOSED states, but the
implementation at line 149 defines `SOCKET_ACTIVE_STATES = [WebSocket.OPEN,
WebSocket.CONNECTING]`. CONNECTING (readyState=0) is also considered active.
Add a test case with `setReadyState(ws, WebSocket.CONNECTING)` to verify
`isSocketActive` returns true for that state.
</div>
</div>
<small><i>Code Review Run #469316</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
