diff --git a/packages/socket-mode/src/SocketModeClient.ts b/packages/socket-mode/src/SocketModeClient.ts index 90dec7176..a136ca8e6 100644 --- a/packages/socket-mode/src/SocketModeClient.ts +++ b/packages/socket-mode/src/SocketModeClient.ts @@ -49,11 +49,10 @@ enum Event { WebSocketOpen = 'websocket open', WebSocketClose = 'websocket close', ServerHello = 'server hello', - ServerDisconnectWarning = 'server disconnect warning', - ServerDisconnectOldSocket = 'server disconnect old socket', + ServerExplicitDisconnect = 'server explicit disconnect', ServerPingsNotReceived = 'server pings not received', ServerPongsNotReceived = 'server pongs not received', - ExplicitDisconnect = 'explicit disconnect', + ClientExplicitDisconnect = 'client explicit disconnect', UnableToSocketModeStart = 'unable_to_socket_mode_start', } @@ -170,7 +169,7 @@ export class SocketModeClient extends EventEmitter { } }); // Delegate behavior to state machine - this.stateMachine.handle(Event.ExplicitDisconnect); + this.stateMachine.handle(Event.ClientExplicitDisconnect); }); } @@ -262,6 +261,8 @@ export class SocketModeClient extends EventEmitter { .initialState(State.Disconnected) .on(Event.Start) .transitionTo(State.Connecting) + .on(Event.ClientExplicitDisconnect) + .transitionTo(State.Disconnected) .state(State.Connecting) .onEnter(() => { this.logger.info('Going to establish a new connection to Slack ...'); @@ -272,7 +273,7 @@ export class SocketModeClient extends EventEmitter { .on(Event.WebSocketClose) .transitionTo(State.Reconnecting).withCondition(this.autoReconnectCondition.bind(this)) .transitionTo(State.Disconnecting) - .on(Event.ExplicitDisconnect) + .on(Event.ClientExplicitDisconnect) .transitionTo(State.Disconnecting) .on(Event.Failure) .transitionTo(State.Disconnected) @@ -290,7 +291,7 @@ export class SocketModeClient extends EventEmitter { .withCondition(this.autoReconnectCondition.bind(this)) .withAction(() => this.markCurrentWebSocketAsInactive()) .transitionTo(State.Disconnecting) - .on(Event.ExplicitDisconnect) + .on(Event.ClientExplicitDisconnect) .transitionTo(State.Disconnecting) .withAction(() => this.markCurrentWebSocketAsInactive()) .on(Event.ServerPingsNotReceived) @@ -299,10 +300,7 @@ export class SocketModeClient extends EventEmitter { .on(Event.ServerPongsNotReceived) .transitionTo(State.Reconnecting).withCondition(this.autoReconnectCondition.bind(this)) .transitionTo(State.Disconnecting) - .on(Event.ServerDisconnectWarning) - .transitionTo(State.Reconnecting).withCondition(this.autoReconnectCondition.bind(this)) - .transitionTo(State.Disconnecting) - .on(Event.ServerDisconnectOldSocket) + .on(Event.ServerExplicitDisconnect) .transitionTo(State.Reconnecting).withCondition(this.autoReconnectCondition.bind(this)) .transitionTo(State.Disconnecting) .onExit(() => { @@ -736,17 +734,10 @@ export class SocketModeClient extends EventEmitter { return; } - // Open the second WebSocket connection in preparation for the existing WebSocket disconnecting - if (event.type === 'disconnect' && event.reason === 'warning') { - this.logger.debug('Received "disconnect" (warning) message - creating the second connection'); - this.stateMachine.handle(Event.ServerDisconnectWarning); - return; - } - - // Close the primary WebSocket in favor of secondary WebSocket, assign secondary to primary - if (event.type === 'disconnect' && event.reason === 'refresh_requested') { - this.logger.debug('Received "disconnect" (refresh requested) message - closing the old WebSocket connection'); - this.stateMachine.handle(Event.ServerDisconnectOldSocket); + if (event.type === 'disconnect') { + // Refresh the WebSocket connection when prompted by Slack backend + this.logger.debug(`Received "disconnect" (reason: ${event.reason}) message - will ${this.autoReconnectEnabled ? 'attempt reconnect' : 'disconnect (due to autoReconnectEnabled=false)'}`); + this.stateMachine.handle(Event.ServerExplicitDisconnect); return; } diff --git a/packages/socket-mode/test/integration.spec.js b/packages/socket-mode/test/integration.spec.js index b85197bd0..396141db8 100644 --- a/packages/socket-mode/test/integration.spec.js +++ b/packages/socket-mode/test/integration.spec.js @@ -17,9 +17,12 @@ let exposed_ws_connection = null; // Socket mode client pointing to the above two posers let client = null; + +const DISCONNECT_REASONS = ['warning', 'refresh_requested', 'too_many_websockets']; + describe('Integration tests with a WebSocket server', () => { beforeEach(() => { - server = createServer((req, res) => { + server = createServer((_req, res) => { res.writeHead(200, { 'content-type': 'application/json' }); res.end(JSON.stringify({ ok: true, @@ -55,6 +58,9 @@ describe('Integration tests with a WebSocket server', () => { await client.start(); await client.disconnect(); }); + it('can call `disconnect()` even if already disconnected without issue', async () => { + await client.disconnect(); + }); it('can listen on random event types and receive payload properties', async () => { client.on('connected', () => { exposed_ws_connection.send(JSON.stringify({ @@ -82,7 +88,7 @@ describe('Integration tests with a WebSocket server', () => { // Shut down the main WS-endpoint-retrieval server - we will customize its behaviour for this test server.close(); let num_attempts = 0; - server = createServer((req, res) => { + server = createServer((_req, res) => { num_attempts += 1; res.writeHead(200, { 'content-type': 'application/json' }); if (num_attempts < 3) { @@ -103,7 +109,7 @@ describe('Integration tests with a WebSocket server', () => { await client.disconnect(); }); }); - describe('failure modes / unexpected messages sent to client', () => { + describe('failure modes / unexpected messages sent to client', () => { let debugLoggerSpy = sinon.stub(); const noop = () => {}; beforeEach(() => { @@ -172,14 +178,6 @@ describe('Integration tests with a WebSocket server', () => { await client.disconnect(); assert.isTrue(raised); }); - it.skip('raises reconnecting event after `disconnect()`', async () => { - // TODO: doesnt seem to work, maybe need to set up client with reconnecting configuration - let raised = false; - client.on('reconnecting', () => { raised = true; }); - await client.start(); - await client.disconnect(); - assert.isTrue(raised); - }); it('raises disconnected event after `disconnect()`', async () => { let raised = false; client.on('disconnected', () => { raised = true; }); @@ -187,6 +185,60 @@ describe('Integration tests with a WebSocket server', () => { await client.disconnect(); assert.isTrue(raised); }); + describe('slack_event', () => { + beforeEach(() => { + // Disable auto reconnect for these tests + client = new SocketModeClient({ appToken: 'whatever', logLevel: LogLevel.ERROR, autoReconnectEnabled: false, clientOptions: { + slackApiUrl: `http://localhost:${HTTP_PORT}/` + }}); + }); + afterEach(async () => { + await client.disconnect(); + }); + // These tests check that specific type:disconnect events, of various reasons, sent by Slack backend are not raised as slack_events for apps to consume. + DISCONNECT_REASONS.forEach((reason) => { + it(`should not raise a type:disconnect reason:${reason} message as a slack_event`, async () => { + let raised = false; + client.on('connected', () => { + exposed_ws_connection.send(JSON.stringify({type:'disconnect', reason})); + }); + client.on('slack_event', () => { + raised = true; + }); + await client.start(); + await sleep(10); + assert.isFalse(raised); + }); + }); + }); + describe('including reconnection ability', () => { + it('raises reconnecting event after peer disconnects underlying WS connection', async () => { + const reconnectingWaiter = new Promise((res) => client.on('reconnecting', res)); + await client.start(); + // force a WS disconnect from the server + exposed_ws_connection.terminate(); + // create another waiter for post-reconnect connected event + const reconnectedWaiter = new Promise((res) => client.on('connected', res)); + // if we pass the point where the reconnectingWaiter succeeded, then we have verified the reconnecting event is raised + // and this test can be considered passing. if we time out here, then that is an indication of a failure. + await reconnectingWaiter; + await reconnectedWaiter; // wait for this to ensure we dont raise an unexpected error by calling `disconnect` mid-reconnect. + await client.disconnect(); + }); + DISCONNECT_REASONS.forEach((reason) => { + it(`should reconnect gracefully if server sends a disconnect (reason: ${reason}) message`, async () => { + await client.start(); + // force a WS disconnect from the server + exposed_ws_connection.send(JSON.stringify({type:"disconnect", reason})); + // create a waiter for post-reconnect connected event + const reconnectedWaiter = new Promise((res) => client.on('connected', res)); + // if we pass the point where the reconnectedWaiter succeeded, then we have verified the reconnection succeeded + // and this test can be considered passing. if we time out here, then that is an indication of a failure. + await reconnectedWaiter; + await client.disconnect(); + }); + }); + }); }); });