From 5644850c0159abf4b36bebc60ea4f6bd20bd759c Mon Sep 17 00:00:00 2001 From: Sylvain Lesage Date: Thu, 23 Jan 2020 11:30:20 +0100 Subject: [PATCH] feat: remove user from list on disconnection Should resolves the problem of "ghosts" users. In particular, see https://socket.io/docs/client-api/#new-Manager-url-options > timeout | 20000 | connection timeout before a connect_error and > | | connect_timeout events are emitted The client should be disconnected after the timeout, and the user should be removed from the list (not tested - don't know how to simulate this) --- .../events/toserver/disconnect.event.spec.ts | 10 +++ .../events/toserver/disconnect.event.ts | 5 ++ src/domain/events/toserver/event.to.server.ts | 2 +- src/domain/events/toserver/index.ts | 2 +- src/socket.io/socket.spec.ts | 84 +++++++++++++++++++ src/socket.io/socket.ts | 19 +++++ 6 files changed, 120 insertions(+), 2 deletions(-) create mode 100644 src/domain/events/toserver/disconnect.event.spec.ts create mode 100644 src/domain/events/toserver/disconnect.event.ts diff --git a/src/domain/events/toserver/disconnect.event.spec.ts b/src/domain/events/toserver/disconnect.event.spec.ts new file mode 100644 index 0000000..50ccbcc --- /dev/null +++ b/src/domain/events/toserver/disconnect.event.spec.ts @@ -0,0 +1,10 @@ +import { expect } from 'chai' +import { DisconnectEvent } from './disconnect.event' + +describe('Events', () => { + describe('DisconnectEvent', () => { + it("should have 'disconnect' event as name", () => { + expect(DisconnectEvent.eventName).to.equal('disconnect') + }) + }) +}) diff --git a/src/domain/events/toserver/disconnect.event.ts b/src/domain/events/toserver/disconnect.event.ts new file mode 100644 index 0000000..ebf5d21 --- /dev/null +++ b/src/domain/events/toserver/disconnect.event.ts @@ -0,0 +1,5 @@ +import { EventToServer } from './event.to.server' + +export class DisconnectEvent { + static readonly eventName: string = EventToServer.Disconnect +} diff --git a/src/domain/events/toserver/event.to.server.ts b/src/domain/events/toserver/event.to.server.ts index 2e75571..1a73562 100644 --- a/src/domain/events/toserver/event.to.server.ts +++ b/src/domain/events/toserver/event.to.server.ts @@ -1,6 +1,6 @@ export class EventToServer { static readonly Connect = 'connect' - // static readonly Disconnection = 'disconnection' + static readonly Disconnect = 'disconnect' // static readonly JoinRoom = 'join-room' // static readonly LeaveRoom = 'leave-room' diff --git a/src/domain/events/toserver/index.ts b/src/domain/events/toserver/index.ts index d79b9ef..4650942 100644 --- a/src/domain/events/toserver/index.ts +++ b/src/domain/events/toserver/index.ts @@ -1,5 +1,5 @@ export * from './connect.event' -// export * from './disconnection.event' +export * from './disconnect.event' // export * from './join.room.event' // export * from "./leave.room.event"; diff --git a/src/socket.io/socket.spec.ts b/src/socket.io/socket.spec.ts index 2aedefd..a475233 100644 --- a/src/socket.io/socket.spec.ts +++ b/src/socket.io/socket.spec.ts @@ -118,6 +118,90 @@ describe('Server', () => { }) }) + describe('disconnect', () => { + let newClient: SocketIOClient.Socket + let newClientId: SocketIOClient.Socket['id'] + let getUsersList: Promise + + beforeEach(async () => { + newClient = ioClient.connect(socketUrl + '/occupapp-beta', options) + // Note: the newClient.id must be polled after the 'connect' event has + // been received + await new Promise(resolve => newClient.on('connect', resolve)) + newClientId = newClient.id + getUsersList = new Promise(resolve => + client.on(UsersListEvent.eventName, resolve) + ) + }) + + // TODO: test that the user is removed if connection is lost, or comes + // from the server side? + // For example, that the "ghosts" users are removed correctly + // See https://socket.io/docs/client-api/#new-Manager-url-options + // > timeout | 20000 | connection timeout before a connect_error and + // > | | connect_timeout events are emitted + // + // The client should be disconnected after the timeout, and the user + // should be removed from the list + // Is it possible to simulate this in test? See socket.io integration + // tests for reference + it('should disconnect socket', async () => { + // act + newClient.disconnect() + + //assert + expect(newClient.connected).to.be.false + }) + it('should send the ordered list of users', async () => { + // act + newClient.disconnect() + const list = await getUsersList + + // assert + expect(list).to.exist + list.should.all.have.property('name') + list.should.all.have.property('color') + }) + it('should not include the disconnected socket id in the list of users', async () => { + // act + newClient.disconnect() + const list = await getUsersList + + // assert + list.should.not.include.something.that.has.property('id', newClientId) + }) + it('should include the other connected users', async () => { + // act + newClient.disconnect() + const list = await getUsersList + + // assert + expect(list).to.have.length(2) + expect(list[ACTIVE_CLIENT_IDX]).to.have.property('id', client.id) + expect(list[PASSIVE_CLIENT_IDX]).to.have.property( + 'id', + passiveClient.id + ) + }) + it('should log that the socket has been disconnected and the user has been removed', async () => { + // act + newClient.disconnect() + await getUsersList + + // assert + mockLogger + .getInfoLogs() + .should.include.something.that.equals( + `Disconnection from socket ${newClientId} - reason: client namespace disconnect` + ) + mockLogger + .getInfoLogs() + .should.include.something.that.equals( + `removeUser - User removed (client socket ${newClientId})` + ) + }) + }) + describe('update-state', () => { let updateState: ( args: UpdateStateEventArgs diff --git a/src/socket.io/socket.ts b/src/socket.io/socket.ts index 973ba09..b14b49d 100644 --- a/src/socket.io/socket.ts +++ b/src/socket.io/socket.ts @@ -3,6 +3,7 @@ import { Constants } from './constants' import { Guard, ConsoleLogger } from '../shared/index' import { ConnectEvent, + DisconnectEvent, UpdateStateEvent, UpdateUserNameEvent, UpdateUserColorEvent, @@ -29,6 +30,16 @@ class Socket { this.emitUsersListToAll() this.emitStateToUser(socket) + socket.on(DisconnectEvent.eventName, (reason: string) => { + this.log.info( + `Disconnection from socket ${socket.id} - reason: ${reason}` + ) + if (this.removeUser(socket.id)) { + // TODO: alternative: send mutation : user-removed + this.emitUsersListToAll() + } + }) + socket.on( UpdateUserNameEvent.eventName, (data: UpdateUserNameEventArgs, ack: UpdateUserNameAck) => { @@ -139,6 +150,14 @@ class Socket { return newUser } + private removeUser = (id: SocketIOClient.Socket['id']): boolean => { + const removed: boolean = this.users.delete(id) + if (removed) { + this.log.info(`removeUser`, `User removed (client socket ${id})`) + } + return removed + } + // private emitInternalServerError(socket: SocketIOClient.Socket, error: Error) { // let internalServerErrorEvent = new InternalServerErrorEvent(error) // socket.emit(