From 478a727c615dd61577aac27a40b5c4bf2b4d42b7 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 29 Jul 2019 15:19:08 +0100 Subject: [PATCH] ignore spurious requests to leave other docs --- .../real-time/app/coffee/RoomManager.coffee | 15 ++++++- .../acceptance/coffee/LeaveDocTests.coffee | 12 ++++++ .../test/unit/coffee/RoomManagerTests.coffee | 41 ++++++++++++++++++- 3 files changed, 66 insertions(+), 2 deletions(-) diff --git a/services/real-time/app/coffee/RoomManager.coffee b/services/real-time/app/coffee/RoomManager.coffee index 0d8a8aaf06..27d69ca379 100644 --- a/services/real-time/app/coffee/RoomManager.coffee +++ b/services/real-time/app/coffee/RoomManager.coffee @@ -32,7 +32,9 @@ module.exports = RoomManager = # channel subscriptions... but it will be safer if we leave them # explicitly, and then socket.io will just regard this as a client that # has not joined any rooms and do a final disconnection. - for id in @_roomsClientIsIn(client) + roomsToLeave = @_roomsClientIsIn(client) + logger.log {client: client.id, roomsToLeave: roomsToLeave}, "client leaving project" + for id in roomsToLeave entity = IdMap.get(id) @leaveEntity client, entity, id @@ -66,6 +68,12 @@ module.exports = RoomManager = callback() leaveEntity: (client, entity, id) -> + # Ignore any requests to leave when the client is not actually in the + # room. This can happen if the client sends spurious leaveDoc requests + # for old docs after a reconnection. + if !@_clientAlreadyInRoom(client, id) + logger.warn {client: client.id, entity, id}, "ignoring request from client to leave room it is not in" + return client.leave id afterCount = @_clientsInRoom(client, id) logger.log {client: client.id, entity, id, afterCount}, "client left room" @@ -93,3 +101,8 @@ module.exports = RoomManager = [prefix, room] = fullRoomPath.split('/', 2) room return roomList + + _clientAlreadyInRoom: (client, room) -> + nsp = client.namespace.name + name = (nsp + '/') + room; + return client.manager.roomClients?[client.id]?[name] \ No newline at end of file diff --git a/services/real-time/test/acceptance/coffee/LeaveDocTests.coffee b/services/real-time/test/acceptance/coffee/LeaveDocTests.coffee index e4875fcbe5..e68b111c64 100644 --- a/services/real-time/test/acceptance/coffee/LeaveDocTests.coffee +++ b/services/real-time/test/acceptance/coffee/LeaveDocTests.coffee @@ -16,9 +16,12 @@ describe "leaveDoc", -> @version = 42 @ops = ["mock", "doc", "ops"] sinon.spy(logger, "error") + sinon.spy(logger, "warn") + @other_doc_id = FixturesManager.getRandomId() after -> logger.error.restore() # remove the spy + logger.warn.restore() describe "when joined to a doc", -> beforeEach (done) -> @@ -70,3 +73,12 @@ describe "leaveDoc", -> RealTimeClient.getConnectedClient @client.socket.sessionid, (error, client) => expect(@doc_id in client.rooms).to.equal false done() + + describe "when sending a leaveDoc for a room the client has not joined ", -> + beforeEach (done) -> + @client.emit "leaveDoc", @other_doc_id, (error) -> + throw error if error? + done() + + it "should trigger a warning only", -> + sinon.assert.calledWith(logger.warn, sinon.match.any, "ignoring request from client to leave room it is not in") \ No newline at end of file diff --git a/services/real-time/test/unit/coffee/RoomManagerTests.coffee b/services/real-time/test/unit/coffee/RoomManagerTests.coffee index ee46a3ef04..fc8375ae54 100644 --- a/services/real-time/test/unit/coffee/RoomManagerTests.coffee +++ b/services/real-time/test/unit/coffee/RoomManagerTests.coffee @@ -12,9 +12,10 @@ describe 'RoomManager', -> @client = {namespace: {name: ''}, id: "first-client"} @RoomManager = SandboxedModule.require modulePath, requires: "settings-sharelatex": @settings = {} - "logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub() } + "logger-sharelatex": @logger = { log: sinon.stub(), warn: sinon.stub(), error: sinon.stub() } "metrics-sharelatex": @metrics = { gauge: sinon.stub() } @RoomManager._clientsInRoom = sinon.stub() + @RoomManager._clientAlreadyInRoom = sinon.stub() @RoomEvents = @RoomManager.eventSource() sinon.spy(@RoomEvents, 'emit') sinon.spy(@RoomEvents, 'once') @@ -112,6 +113,9 @@ describe 'RoomManager', -> describe "when doc room will be empty after this client has left", -> beforeEach -> + @RoomManager._clientAlreadyInRoom + .withArgs(@client, @doc_id) + .returns(true) @RoomManager._clientsInRoom .withArgs(@client, @doc_id) .onCall(0).returns(0) @@ -128,6 +132,9 @@ describe 'RoomManager', -> describe "when there are other clients in the doc room", -> beforeEach -> + @RoomManager._clientAlreadyInRoom + .withArgs(@client, @doc_id) + .returns(true) @RoomManager._clientsInRoom .withArgs(@client, @doc_id) .onCall(0).returns(123) @@ -140,6 +147,24 @@ describe 'RoomManager', -> it "should not emit any events", -> @RoomEvents.emit.called.should.equal false + describe "when the client is not in the doc room", -> + + beforeEach -> + @RoomManager._clientAlreadyInRoom + .withArgs(@client, @doc_id) + .returns(false) + @RoomManager._clientsInRoom + .withArgs(@client, @doc_id) + .onCall(0).returns(0) + @client.leave = sinon.stub() + @RoomManager.leaveDoc @client, @doc_id + + it "should not leave the room", -> + @client.leave.called.should.equal false + + it "should not emit any events", -> + @RoomEvents.emit.called.should.equal false + describe "leaveProjectAndDocs", -> @@ -167,6 +192,13 @@ describe 'RoomManager', -> .withArgs(@client, @project_id) .onCall(0).returns(0) .onCall(1).returns(0) + @RoomManager._clientAlreadyInRoom + .withArgs(@client, @doc_id) + .returns(true) + .withArgs(@client, @other_doc_id) + .returns(true) + .withArgs(@client, @project_id) + .returns(true) @RoomEvents.on 'project-active', (id) => setTimeout () => @RoomEvents.emit "project-subscribed-#{id}" @@ -212,6 +244,13 @@ describe 'RoomManager', -> .withArgs(@client, @project_id) .onFirstCall().returns(123) .onSecondCall().returns(122) + @RoomManager._clientAlreadyInRoom + .withArgs(@client, @doc_id) + .returns(true) + .withArgs(@client, @other_doc_id) + .returns(true) + .withArgs(@client, @project_id) + .returns(true) @RoomManager.leaveProjectAndDocs @client it "should leave all the docs", ->