rusackas commented on code in PR #40856:
URL: https://github.com/apache/superset/pull/40856#discussion_r3382276249
##########
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:
Good catch. Added in d10e3a6ebe: the `isSocketActive` test now asserts that
a socket in the `CONNECTING` readyState is also reported active (matching
`SOCKET_ACTIVE_STATES = [WebSocket.OPEN, WebSocket.CONNECTING]`), alongside the
existing OPEN/CLOSED assertions.
--
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]
