From 55af5e502ffed9ba2d41ef515a840aa28cf19af8 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Tue, 12 May 2020 12:55:04 +0200 Subject: [PATCH] [WebsocketController] skip leaveProject when joinProject didn't complete Also drop dead code: - user_id bailout There is a check on a completed joinProject call now. It will always set a user_id, see Router.coffee which has a fallback `{_id:"..."}`. - late project_id bailout WebsocketLoadBalancer.emitToRoom will not work without a project_id. We have to bail out before the call. --- .../app/coffee/WebsocketController.coffee | 13 ++---------- .../coffee/WebsocketControllerTests.coffee | 21 +++++++++++++++---- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/services/real-time/app/coffee/WebsocketController.coffee b/services/real-time/app/coffee/WebsocketController.coffee index 786a77d114..35f572c856 100644 --- a/services/real-time/app/coffee/WebsocketController.coffee +++ b/services/real-time/app/coffee/WebsocketController.coffee @@ -51,23 +51,14 @@ module.exports = WebsocketController = # is determined by FLUSH_IF_EMPTY_DELAY. FLUSH_IF_EMPTY_DELAY: 500 #ms leaveProject: (io, client, callback = (error) ->) -> - metrics.inc "editor.leave-project" Utils.getClientAttributes client, ["project_id", "user_id"], (error, {project_id, user_id}) -> return callback(error) if error? + return callback() unless project_id # client did not join project + metrics.inc "editor.leave-project" logger.log {project_id, user_id, client_id: client.id}, "client leaving project" WebsocketLoadBalancer.emitToRoom project_id, "clientTracking.clientDisconnected", client.id - # bail out if the client had not managed to authenticate or join - # the project. Prevents downstream errors in docupdater from - # flushProjectToMongoAndDelete with null project_id. - if not user_id? - logger.log {client_id: client.id}, "client leaving, unknown user" - return callback() - if not project_id? - logger.log {user_id: user_id, client_id: client.id}, "client leaving, not in project" - return callback() - # We can do this in the background ConnectedUsersManager.markUserAsDisconnected project_id, client.id, (err) -> if err? diff --git a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee index 62243f797c..ac5e9f76fd 100644 --- a/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee +++ b/services/real-time/test/unit/coffee/WebsocketControllerTests.coffee @@ -143,6 +143,19 @@ describe 'WebsocketController', -> @WebsocketController.FLUSH_IF_EMPTY_DELAY = 0 tk.reset() # Allow setTimeout to work. + describe "when the client did not joined a project yet", -> + beforeEach (done) -> + @client.params = {} + @WebsocketController.leaveProject @io, @client, done + + it "should bail out when calling leaveProject", () -> + @WebsocketLoadBalancer.emitToRoom.called.should.equal false + @RoomManager.leaveProjectAndDocs.called.should.equal false + @ConnectedUsersManager.markUserAsDisconnected.called.should.equal false + + it "should not inc any metric", () -> + @metrics.inc.called.should.equal false + describe "when the project is empty", -> beforeEach (done) -> @clientsInRoom = [] @@ -201,8 +214,8 @@ describe 'WebsocketController', -> .calledWith(@project_id) .should.equal false - it "should increment the leave-project metric", -> - @metrics.inc.calledWith("editor.leave-project").should.equal true + it "should not increment the leave-project metric", -> + @metrics.inc.calledWith("editor.leave-project").should.equal false describe "when client has not joined a project", -> beforeEach (done) -> @@ -225,8 +238,8 @@ describe 'WebsocketController', -> .calledWith(@project_id) .should.equal false - it "should increment the leave-project metric", -> - @metrics.inc.calledWith("editor.leave-project").should.equal true + it "should not increment the leave-project metric", -> + @metrics.inc.calledWith("editor.leave-project").should.equal false describe "joinDoc", -> beforeEach ->