Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

Commit

Permalink
feat: remove user from list on disconnection
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
severo committed Jan 23, 2020
1 parent 3002881 commit 5644850
Show file tree
Hide file tree
Showing 6 changed files with 120 additions and 2 deletions.
10 changes: 10 additions & 0 deletions src/domain/events/toserver/disconnect.event.spec.ts
Original file line number Diff line number Diff line change
@@ -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')
})
})
})
5 changes: 5 additions & 0 deletions src/domain/events/toserver/disconnect.event.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { EventToServer } from './event.to.server'

export class DisconnectEvent {
static readonly eventName: string = EventToServer.Disconnect
}
2 changes: 1 addition & 1 deletion src/domain/events/toserver/event.to.server.ts
Original file line number Diff line number Diff line change
@@ -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'
Expand Down
2 changes: 1 addition & 1 deletion src/domain/events/toserver/index.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down
84 changes: 84 additions & 0 deletions src/socket.io/socket.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,90 @@ describe('Server', () => {
})
})

describe('disconnect', () => {
let newClient: SocketIOClient.Socket
let newClientId: SocketIOClient.Socket['id']
let getUsersList: Promise<ExportedUser[]>

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
Expand Down
19 changes: 19 additions & 0 deletions src/socket.io/socket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Constants } from './constants'
import { Guard, ConsoleLogger } from '../shared/index'
import {
ConnectEvent,
DisconnectEvent,
UpdateStateEvent,
UpdateUserNameEvent,
UpdateUserColorEvent,
Expand All @@ -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) => {
Expand Down Expand Up @@ -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(
Expand Down

0 comments on commit 5644850

Please sign in to comment.