Skip to content

Commit dc48959

Browse files
author
Fil Maj
authored
socket-mode: do not throw if calling disconnect() and already disconnected, and do not raise slack_event if message received is of type: disconnect
1 parent 4008057 commit dc48959

File tree

3 files changed

+265
-29
lines changed

3 files changed

+265
-29
lines changed

packages/socket-mode/package.json

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@slack/socket-mode",
3-
"version": "1.3.4",
3+
"version": "1.3.5-rc.1",
44
"description": "Official library for using the Slack Platform's Socket Mode API",
55
"author": "Slack Technologies, LLC",
66
"license": "MIT",
@@ -40,19 +40,17 @@
4040
"build": "npm run build:clean && tsc",
4141
"build:clean": "shx rm -rf ./dist ./coverage ./.nyc_output",
4242
"lint": "eslint --ext .ts src",
43-
"test": "npm run lint && npm run build && nyc mocha --config .mocharc.json src/*.spec.js",
43+
"test": "npm run lint && npm run build && nyc mocha --config .mocharc.json src/*.spec.js && npm run test:integration",
44+
"test:integration": "mocha --config .mocharc.json test/integration.spec.js",
4445
"watch": "npx nodemon --watch 'src' --ext 'ts' --exec npm run build"
4546
},
4647
"dependencies": {
4748
"@slack/logger": "^3.0.0",
4849
"@slack/web-api": "^6.11.2",
4950
"@types/node": ">=12.0.0",
50-
"@types/p-queue": "^2.3.2",
5151
"@types/ws": "^7.4.7",
52-
"eventemitter3": "^3.1.0",
52+
"eventemitter3": "^5",
5353
"finity": "^0.5.4",
54-
"p-cancelable": "^1.1.0",
55-
"p-queue": "^2.4.2",
5654
"ws": "^7.5.3"
5755
},
5856
"devDependencies": {

packages/socket-mode/src/SocketModeClient.ts

Lines changed: 16 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -49,11 +49,10 @@ enum Event {
4949
WebSocketOpen = 'websocket open',
5050
WebSocketClose = 'websocket close',
5151
ServerHello = 'server hello',
52-
ServerDisconnectWarning = 'server disconnect warning',
53-
ServerDisconnectOldSocket = 'server disconnect old socket',
52+
ServerExplicitDisconnect = 'server explicit disconnect',
5453
ServerPingsNotReceived = 'server pings not received',
5554
ServerPongsNotReceived = 'server pongs not received',
56-
ExplicitDisconnect = 'explicit disconnect',
55+
ClientExplicitDisconnect = 'client explicit disconnect',
5756
UnableToSocketModeStart = 'unable_to_socket_mode_start',
5857
}
5958

@@ -170,7 +169,7 @@ export class SocketModeClient extends EventEmitter {
170169
}
171170
});
172171
// Delegate behavior to state machine
173-
this.stateMachine.handle(Event.ExplicitDisconnect);
172+
this.stateMachine.handle(Event.ClientExplicitDisconnect);
174173
});
175174
}
176175

@@ -202,15 +201,17 @@ export class SocketModeClient extends EventEmitter {
202201
.transitionTo(ConnectingState.Reconnecting).withCondition(this.reconnectingCondition.bind(this))
203202
.transitionTo(ConnectingState.Failed)
204203
.state(ConnectingState.Reconnecting)
205-
.do(async () => {
204+
.do(() => new Promise((res, _rej) => {
206205
// Trying to reconnect after waiting for a bit...
207206
this.numOfConsecutiveReconnectionFailures += 1;
208207
const millisBeforeRetry = this.clientPingTimeoutMillis * this.numOfConsecutiveReconnectionFailures;
209208
this.logger.debug(`Before trying to reconnect, this client will wait for ${millisBeforeRetry} milliseconds`);
210209
setTimeout(() => {
211210
this.emit(ConnectingState.Authenticating);
211+
res(true);
212212
}, millisBeforeRetry);
213-
})
213+
}))
214+
.onSuccess().transitionTo(ConnectingState.Authenticating)
214215
.onFailure().transitionTo(ConnectingState.Failed)
215216
.state(ConnectingState.Authenticated)
216217
.onEnter(this.configureAuthenticatedWebSocket.bind(this))
@@ -261,6 +262,8 @@ export class SocketModeClient extends EventEmitter {
261262
.initialState(State.Disconnected)
262263
.on(Event.Start)
263264
.transitionTo(State.Connecting)
265+
.on(Event.ClientExplicitDisconnect)
266+
.transitionTo(State.Disconnected)
264267
.state(State.Connecting)
265268
.onEnter(() => {
266269
this.logger.info('Going to establish a new connection to Slack ...');
@@ -271,7 +274,7 @@ export class SocketModeClient extends EventEmitter {
271274
.on(Event.WebSocketClose)
272275
.transitionTo(State.Reconnecting).withCondition(this.autoReconnectCondition.bind(this))
273276
.transitionTo(State.Disconnecting)
274-
.on(Event.ExplicitDisconnect)
277+
.on(Event.ClientExplicitDisconnect)
275278
.transitionTo(State.Disconnecting)
276279
.on(Event.Failure)
277280
.transitionTo(State.Disconnected)
@@ -289,7 +292,7 @@ export class SocketModeClient extends EventEmitter {
289292
.withCondition(this.autoReconnectCondition.bind(this))
290293
.withAction(() => this.markCurrentWebSocketAsInactive())
291294
.transitionTo(State.Disconnecting)
292-
.on(Event.ExplicitDisconnect)
295+
.on(Event.ClientExplicitDisconnect)
293296
.transitionTo(State.Disconnecting)
294297
.withAction(() => this.markCurrentWebSocketAsInactive())
295298
.on(Event.ServerPingsNotReceived)
@@ -298,10 +301,7 @@ export class SocketModeClient extends EventEmitter {
298301
.on(Event.ServerPongsNotReceived)
299302
.transitionTo(State.Reconnecting).withCondition(this.autoReconnectCondition.bind(this))
300303
.transitionTo(State.Disconnecting)
301-
.on(Event.ServerDisconnectWarning)
302-
.transitionTo(State.Reconnecting).withCondition(this.autoReconnectCondition.bind(this))
303-
.transitionTo(State.Disconnecting)
304-
.on(Event.ServerDisconnectOldSocket)
304+
.on(Event.ServerExplicitDisconnect)
305305
.transitionTo(State.Reconnecting).withCondition(this.autoReconnectCondition.bind(this))
306306
.transitionTo(State.Disconnecting)
307307
.onExit(() => {
@@ -731,17 +731,10 @@ export class SocketModeClient extends EventEmitter {
731731
return;
732732
}
733733

734-
// Open the second WebSocket connection in preparation for the existing WebSocket disconnecting
735-
if (event.type === 'disconnect' && event.reason === 'warning') {
736-
this.logger.debug('Received "disconnect" (warning) message - creating the second connection');
737-
this.stateMachine.handle(Event.ServerDisconnectWarning);
738-
return;
739-
}
740-
741-
// Close the primary WebSocket in favor of secondary WebSocket, assign secondary to primary
742-
if (event.type === 'disconnect' && event.reason === 'refresh_requested') {
743-
this.logger.debug('Received "disconnect" (refresh requested) message - closing the old WebSocket connection');
744-
this.stateMachine.handle(Event.ServerDisconnectOldSocket);
734+
if (event.type === 'disconnect') {
735+
// Refresh the WebSocket connection when prompted by Slack backend
736+
this.logger.debug(`Received "disconnect" (reason: ${event.reason}) message - will ${this.autoReconnectEnabled ? 'attempt reconnect' : 'disconnect (due to autoReconnectEnabled=false)'}`);
737+
this.stateMachine.handle(Event.ServerExplicitDisconnect);
745738
return;
746739
}
747740

Lines changed: 245 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,245 @@
1+
const { assert } = require('chai');
2+
const { SocketModeClient} = require('../src/SocketModeClient');
3+
const { LogLevel } = require('../src/logger');
4+
const { Server: WebSocketServer } = require('ws');
5+
const { createServer } = require('http');
6+
const sinon = require('sinon');
7+
8+
const HTTP_PORT = 12345;
9+
const WSS_PORT = 23456;
10+
// Basic HTTP server to 'fake' behaviour of `apps.connections.open` endpoint
11+
let server = null;
12+
13+
// Basic WS server to fake slack WS endpoint
14+
let wss = null;
15+
let exposed_ws_connection = null;
16+
17+
// Socket mode client pointing to the above two posers
18+
let client = null;
19+
20+
21+
const DISCONNECT_REASONS = ['warning', 'refresh_requested', 'too_many_websockets'];
22+
23+
describe('Integration tests with a WebSocket server', () => {
24+
beforeEach(() => {
25+
server = createServer((_req, res) => {
26+
res.writeHead(200, { 'content-type': 'application/json' });
27+
res.end(JSON.stringify({
28+
ok: true,
29+
url: `ws://localhost:${WSS_PORT}/`,
30+
}));
31+
});
32+
server.listen(HTTP_PORT);
33+
wss = new WebSocketServer({ port: WSS_PORT });
34+
wss.on('connection', (ws) => {
35+
ws.on('error', (err) => {
36+
assert.fail(err);
37+
});
38+
// Send `Event.ServerHello`
39+
ws.send(JSON.stringify({type: 'hello'}));
40+
exposed_ws_connection = ws;
41+
});
42+
});
43+
afterEach(() => {
44+
server.close();
45+
server = null;
46+
wss.close();
47+
wss = null;
48+
exposed_ws_connection = null;
49+
client = null;
50+
});
51+
describe('establishing connection, receiving valid messages', () => {
52+
beforeEach(() => {
53+
client = new SocketModeClient({ appToken: 'whatever', logLevel: LogLevel.ERROR, clientOptions: {
54+
slackApiUrl: `http://localhost:${HTTP_PORT}/`
55+
}});
56+
});
57+
it('connects to a server via `start()` and gracefully disconnects via `disconnect()`', async () => {
58+
const connected = new Promise((res) => client.once('connected', res)); // due to legacy reasons, await start() does not wait for Connected state, so add additional check here for it
59+
await client.start();
60+
await connected;
61+
await client.disconnect();
62+
});
63+
it('can call `disconnect()` even if already disconnected without issue', async () => {
64+
await client.disconnect();
65+
});
66+
it('can listen on random event types and receive payload properties', async () => {
67+
client.on('connected', () => {
68+
exposed_ws_connection.send(JSON.stringify({
69+
type: 'integration-test',
70+
envelope_id: 12345,
71+
}));
72+
});
73+
await client.start();
74+
await new Promise((res, _rej) => {
75+
client.on('integration-test', (evt) => {
76+
assert.equal(evt.envelope_id, 12345);
77+
res();
78+
});
79+
});
80+
await client.disconnect();
81+
});
82+
});
83+
describe('catastrophic server behaviour', () => {
84+
beforeEach(() => {
85+
client = new SocketModeClient({ appToken: 'whatever', logLevel: LogLevel.ERROR, clientOptions: {
86+
slackApiUrl: `http://localhost:${HTTP_PORT}/`
87+
}, clientPingTimeout: 25});
88+
});
89+
it('should retry if retrieving a WSS URL fails', async () => {
90+
// Shut down the main WS-endpoint-retrieval server - we will customize its behaviour for this test
91+
server.close();
92+
let num_attempts = 0;
93+
server = createServer((_req, res) => {
94+
num_attempts += 1;
95+
res.writeHead(200, { 'content-type': 'application/json' });
96+
if (num_attempts < 3) {
97+
res.end(JSON.stringify({
98+
ok: false,
99+
error: "fatal_error",
100+
}));
101+
} else {
102+
res.end(JSON.stringify({
103+
ok: true,
104+
url: `ws://localhost:${WSS_PORT}/`,
105+
}));
106+
}
107+
});
108+
server.listen(HTTP_PORT);
109+
await client.start();
110+
assert.equal(num_attempts, 3);
111+
await client.disconnect();
112+
});
113+
});
114+
describe('failure modes / unexpected messages sent to client', () => {
115+
let loggerSpy = sinon.stub();
116+
const noop = () => {};
117+
beforeEach(() => {
118+
client = new SocketModeClient({ appToken: 'whatever', clientOptions: {
119+
slackApiUrl: `http://localhost:${HTTP_PORT}/`
120+
}, logger: {
121+
debug: noop,
122+
info: noop,
123+
error: loggerSpy,
124+
getLevel: () => 'error',
125+
}});
126+
});
127+
afterEach(() => {
128+
loggerSpy.resetHistory();
129+
});
130+
it('should error-log that a malformed JSON message was received', async () => {
131+
client.on('connected', () => {
132+
exposed_ws_connection.send('');
133+
});
134+
await client.start();
135+
await sleep(10);
136+
assert.isTrue(loggerSpy.calledWith(sinon.match('Unable to parse an incoming WebSocket message')));
137+
await client.disconnect();
138+
});
139+
});
140+
describe('lifecycle events', () => {
141+
beforeEach(() => {
142+
client = new SocketModeClient({ appToken: 'whatever', logLevel: LogLevel.ERROR, clientOptions: {
143+
slackApiUrl: `http://localhost:${HTTP_PORT}/`
144+
}});
145+
});
146+
it('raises connecting event during `start()`', async () => {
147+
let raised = false;
148+
client.on('connecting', () => { raised = true; });
149+
const connected = new Promise((res) => client.once('connected', res)); // due to legacy reasons, await start() does not wait for Connected state, so add additional check here for it
150+
await client.start();
151+
await connected;
152+
assert.isTrue(raised);
153+
await client.disconnect();
154+
});
155+
it('raises authenticated event during `start()`', async () => {
156+
let raised = false;
157+
client.on('authenticated', () => { raised = true; });
158+
const connected = new Promise((res) => client.once('connected', res)); // due to legacy reasons, await start() does not wait for Connected state, so add additional check here for it
159+
await client.start();
160+
await connected;
161+
assert.isTrue(raised);
162+
await client.disconnect();
163+
});
164+
it('raises disconnecting event during `disconnect()`', async () => {
165+
let raised = false;
166+
const connected = new Promise((res) => client.once('connected', res)); // due to legacy reasons, await start() does not wait for Connected state, so add additional check here for it
167+
client.on('disconnecting', () => { raised = true; });
168+
await client.start();
169+
await connected;
170+
await client.disconnect();
171+
assert.isTrue(raised);
172+
});
173+
it('raises disconnected event after `disconnect()`', async () => {
174+
let raised = false;
175+
const connected = new Promise((res) => client.once('connected', res)); // due to legacy reasons, await start() does not wait for Connected state, so add additional check here for it
176+
client.on('disconnected', () => { raised = true; });
177+
await client.start();
178+
await connected;
179+
await client.disconnect();
180+
assert.isTrue(raised);
181+
});
182+
describe('slack_event', () => {
183+
beforeEach(() => {
184+
// Disable auto reconnect for these tests
185+
client = new SocketModeClient({ appToken: 'whatever', logLevel: LogLevel.ERROR, autoReconnectEnabled: false, clientOptions: {
186+
slackApiUrl: `http://localhost:${HTTP_PORT}/`
187+
}});
188+
});
189+
afterEach(async () => {
190+
await client.disconnect();
191+
});
192+
// 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.
193+
DISCONNECT_REASONS.forEach((reason) => {
194+
it(`should not raise a type:disconnect reason:${reason} message as a slack_event`, async () => {
195+
let raised = false;
196+
client.on('connected', () => {
197+
exposed_ws_connection.send(JSON.stringify({type:'disconnect', reason}));
198+
});
199+
client.on('slack_event', () => {
200+
raised = true;
201+
});
202+
await client.start();
203+
await sleep(10);
204+
assert.isFalse(raised);
205+
});
206+
});
207+
});
208+
describe('including reconnection ability', () => {
209+
it('raises reconnecting event after peer disconnects underlying WS connection', async () => {
210+
const reconnectingWaiter = new Promise((res) => client.on('reconnecting', res));
211+
const connected = new Promise((res) => client.once('connected', res)); // due to legacy reasons, await start() does not wait for Connected state, so add additional check here for it
212+
await client.start();
213+
await connected;
214+
// force a WS disconnect from the server
215+
exposed_ws_connection.terminate();
216+
// create another waiter for post-reconnect connected event
217+
const reconnectedWaiter = new Promise((res) => client.on('connected', res));
218+
// if we pass the point where the reconnectingWaiter succeeded, then we have verified the reconnecting event is raised
219+
// and this test can be considered passing. if we time out here, then that is an indication of a failure.
220+
await reconnectingWaiter;
221+
await reconnectedWaiter; // wait for this to ensure we dont raise an unexpected error by calling `disconnect` mid-reconnect.
222+
await client.disconnect();
223+
});
224+
DISCONNECT_REASONS.forEach((reason) => {
225+
it(`should reconnect gracefully if server sends a disconnect (reason: ${reason}) message`, async () => {
226+
const connected = new Promise((res) => client.once('connected', res)); // due to legacy reasons, await start() does not wait for Connected state, so add additional check here for it
227+
await client.start();
228+
await connected;
229+
// force a WS disconnect from the server
230+
exposed_ws_connection.send(JSON.stringify({type:"disconnect", reason}));
231+
// create a waiter for post-reconnect connected event
232+
const reconnectedWaiter = new Promise((res) => client.on('connected', res));
233+
// if we pass the point where the reconnectedWaiter succeeded, then we have verified the reconnection succeeded
234+
// and this test can be considered passing. if we time out here, then that is an indication of a failure.
235+
await reconnectedWaiter;
236+
await client.disconnect();
237+
});
238+
});
239+
});
240+
});
241+
});
242+
243+
function sleep(ms) {
244+
return new Promise(resolve => setTimeout(resolve, ms));
245+
}

0 commit comments

Comments
 (0)