Merge pull request #68 from overleaf/bg-handle-spurious-leave-doc

ignore spurious requests to leave other docs

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]

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")

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", ->