From 4681f24a8467b36673db90df2dba0e11b24fb9ec Mon Sep 17 00:00:00 2001 From: Tony Anziano Date: Thu, 14 May 2020 15:08:20 -0700 Subject: [PATCH] Fixed issue with websocket connections being closed when deep linking into a conversation. (#2146) --- CHANGELOG.md | 1 + .../main/src/server/webSocketServer.spec.ts | 12 ++- .../app/main/src/server/webSocketServer.ts | 78 ++++++++++--------- 3 files changed, 49 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 64a2ece43..fc225a43b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Fixed - [build] Fixed system dialog on Mac OS warning about being unable to check for malicious code in PR [2135](https://github.com/microsoft/BotFramework-Emulator/pull/2135) +- [client / main] Fixed an issue where starting a conversation via deeplink was initializing the WebSocket server twice and closing previously established connections in PR [2146](https://github.com/microsoft/BotFramework-Emulator/pull/2146) ## v4.8.1 - 2019 - 03 - 18 ## Fixed diff --git a/packages/app/main/src/server/webSocketServer.spec.ts b/packages/app/main/src/server/webSocketServer.spec.ts index bc8abb87a..799378357 100644 --- a/packages/app/main/src/server/webSocketServer.spec.ts +++ b/packages/app/main/src/server/webSocketServer.spec.ts @@ -51,6 +51,7 @@ describe('WebSocketServer', () => { mockCreateServer.mockClear(); mockWSServer.handleUpgrade.mockClear(); mockWSServer.on.mockClear(); + (WebSocketServer as any)._restServer = undefined; }); it('should return the corresponding socket for a conversation id', () => { @@ -65,8 +66,6 @@ describe('WebSocketServer', () => { }); it('should initialize the server', async () => { - const cleanupSpy = jest.spyOn(WebSocketServer, 'cleanup').mockImplementationOnce(() => null); - (WebSocketServer as any)._restServer = {}; (WebSocketServer as any)._servers = {}; (WebSocketServer as any)._sockets = {}; const mockRestServer = { @@ -83,8 +82,13 @@ describe('WebSocketServer', () => { expect(port).toBe(56273); expect(mockRestServer.get).toHaveBeenCalledWith('/ws/:conversationId', jasmine.any(Function)); expect((WebSocketServer as any)._restServer).toEqual(mockRestServer); - expect(cleanupSpy).toHaveBeenCalled(); - cleanupSpy.mockRestore(); + }); + + it('should not re-initialize if already initialized', async () => { + (WebSocketServer as any)._restServer = {}; // server initialized + await WebSocketServer.init(); + + expect(mockCreateServer).not.toHaveBeenCalled(); }); it('should clean up the rest server, web sockets, and web socket servers', () => { diff --git a/packages/app/main/src/server/webSocketServer.ts b/packages/app/main/src/server/webSocketServer.ts index 2a7aaed15..119d5fd88 100644 --- a/packages/app/main/src/server/webSocketServer.ts +++ b/packages/app/main/src/server/webSocketServer.ts @@ -50,48 +50,50 @@ export class WebSocketServer { return this._sockets[conversationId]; } - public static async init(): Promise { - if (this._restServer) { - this.cleanup(); - } - this._restServer = createServer({ handleUpgrades: true, name: 'Emulator-WebSocket-Host' }); - this._restServer.get('/ws/:conversationId', (req: Request, res: Response, next: Next) => { - const conversationId = req.params.conversationId; + /** Initializes the server and returns the port it is listening on, or if already initialized, + * is a no-op. + */ + public static async init(): Promise { + if (!this._restServer) { + this._restServer = createServer({ handleUpgrades: true, name: 'Emulator-WebSocket-Host' }); + this._restServer.get('/ws/:conversationId', (req: Request, res: Response, next: Next) => { + const conversationId = req.params.conversationId; - // initialize a new web socket server for each new conversation - if (conversationId && !this._servers[conversationId]) { - if (!(res as any).claimUpgrade) { - return next(new Error('Connection must upgrade for web sockets.')); - } - const { head, socket } = (res as any).claimUpgrade(); - const wsServer = new WSServer({ - noServer: true, - }); - wsServer.on('connection', (socket, req) => { - this._sockets[conversationId] = socket; - socket.on('close', (code, reason) => { - delete this._servers[conversationId]; - delete this._sockets[conversationId]; + // initialize a new web socket server for each new conversation + if (conversationId && !this._servers[conversationId]) { + if (!(res as any).claimUpgrade) { + return next(new Error('Connection must upgrade for web sockets.')); + } + const { head, socket } = (res as any).claimUpgrade(); + const wsServer = new WSServer({ + noServer: true, }); + wsServer.on('connection', (socket, req) => { + this._sockets[conversationId] = socket; + socket.on('close', (code, reason) => { + delete this._servers[conversationId]; + delete this._sockets[conversationId]; + }); + }); + // upgrade the connection to a ws connection + wsServer.handleUpgrade(req, socket, head, socket => { + wsServer.emit('connection', socket, req); + }); + this._servers[conversationId] = wsServer; + } + }); + // dynamically generate the web socket server port + const port = await new Promise((resolve, reject) => { + this._restServer.once('error', err => reject(err)); + this._restServer.listen(null, () => { + resolve(this._restServer.address().port); }); - // upgrade the connection to a ws connection - wsServer.handleUpgrade(req, socket, head, socket => { - wsServer.emit('connection', socket, req); - }); - this._servers[conversationId] = wsServer; - } - }); - // dynamically generate the web socket server port - const port = await new Promise((resolve, reject) => { - this._restServer.once('error', err => reject(err)); - this._restServer.listen(null, () => { - resolve(this._restServer.address().port); }); - }); - this.port = port; - // eslint-disable-next-line no-console - console.log(`Web Socket host server listening on ${port}...`); - return port; + this.port = port; + // eslint-disable-next-line no-console + console.log(`Web Socket host server listening on ${port}...`); + return port; + } } public static cleanup(): void {