bito-code-review[bot] commented on code in PR #40856:
URL: https://github.com/apache/superset/pull/40856#discussion_r3382277230
##########
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:
<!-- Bito Reply -->
The addition of the test case for the CONNECTING state correctly addresses
the reviewer's suggestion. By including `WebSocket.CONNECTING` in the
`isSocketActive` test, the code now accurately verifies that sockets in that
state are considered active, matching the implementation logic.
**superset-websocket/spec/index.test.ts**
```
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);
});
```
--
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]
