Skip to content

Commit

Permalink
socket-mode: do not throw exception when calling disconnect() and alr…
Browse files Browse the repository at this point in the history 
…eady disconnected; do not raise slack_event in case of type:disconnect messages (#1762)

Fixes #1654
  • Loading branch information
@filmaj
filmaj authored Apr 17, 2024
1 parent 3ed5a59 commit 3ebb6ce
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 32 deletions.
33 changes: 12 additions & 21 deletions packages/socket-mode/src/SocketModeClient.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
}

Expand Down Expand Up @@ -170,7 +169,7 @@ export class SocketModeClient extends EventEmitter {
}
});
// Delegate behavior to state machine
this.stateMachine.handle(Event.ExplicitDisconnect);
this.stateMachine.handle(Event.ClientExplicitDisconnect);
});
}

Expand Down Expand Up @@ -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 ...');
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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(() => {
Expand Down Expand Up @@ -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;
}

Expand Down
74 changes: 63 additions & 11 deletions packages/socket-mode/test/integration.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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) {
Expand All @@ -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(() => {
Expand Down Expand Up @@ -172,21 +178,67 @@ 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; });
await client.start();
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();
});
});
});
});
});

Expand Down

0 comments on commit 3ebb6ce

Please sign in to comment.