Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

socket-mode: do not throw exception when calling disconnect() and already disconnected; do not raise slack_event in case of type:disconnect messages #1762

Merged
merged 4 commits into from
Apr 17, 2024
Merged

socket-mode: do not throw exception when calling disconnect() and already disconnected; do not raise slack_event in case of type:disconnect messages #1762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
afc5e12
socket-mode: add test for reconnecting event via integration tests
Mar 21, 2024
ddfee0c
more integration tests for handling a few of the `disconnect` type me…
Mar 21, 2024
1a3d473
Fixing a couple of bugs:
Apr 1, 2024
58e995b
Refactor internal state machine disconnect events into ServerExplicit…
Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 12 additions & 21 deletions packages/socket-mode/src/SocketModeClient.ts
View file
Open in desktop
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
Open in desktop
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
Loading