MDL-66607 message: Resolve race conditions in message deletion process
authorAndrew Nicols <andrew@nicols.co.uk>
Mon, 9 Mar 2020 01:08:21 +0000 (09:08 +0800)
committerAndrew Nicols <andrew@nicols.co.uk>
Wed, 11 Mar 2020 08:23:16 +0000 (16:23 +0800)
This commit makes several changes:
1) Explicitly stop polling for messages when a conversation is deleted;
2) Check for deleted conversations when displaying new messages;
3) Do not add a new empty conversation; and
4) Introduce pendingJS checks to ensure that Behat waits for messags to finish rendering.

message/amd/build/message_drawer_view_conversation.min.js
message/amd/build/message_drawer_view_conversation.min.js.map
message/amd/build/message_drawer_view_overview_section.min.js
message/amd/build/message_drawer_view_overview_section.min.js.map
message/amd/src/message_drawer_view_conversation.js
message/amd/src/message_drawer_view_overview_section.js

index dddf264..141c6fa 100644 (file)
Binary files a/message/amd/build/message_drawer_view_conversation.min.js and b/message/amd/build/message_drawer_view_conversation.min.js differ
index 32fe99e..1364583 100644 (file)
Binary files a/message/amd/build/message_drawer_view_conversation.min.js.map and b/message/amd/build/message_drawer_view_conversation.min.js.map differ
index c6daf6f..a29c36d 100644 (file)
Binary files a/message/amd/build/message_drawer_view_overview_section.min.js and b/message/amd/build/message_drawer_view_overview_section.min.js differ
index 1331bfd..346dadc 100644 (file)
Binary files a/message/amd/build/message_drawer_view_overview_section.min.js.map and b/message/amd/build/message_drawer_view_overview_section.min.js.map differ
index 7efc94f..b05182c 100644 (file)
@@ -107,6 +107,8 @@ function(
     var isResetting = true;
     // If the UI is currently sending a message.
     var isSendingMessage = false;
+    // If the UI is currently deleting a conversation.
+    var isDeletingConversationContent = false;
     // A buffer of messages to send.
     var sendMessageBuffer = [];
     // These functions which will be generated when this module is
@@ -558,7 +560,7 @@ function(
             var mostRecentMessage = messages.length ? messages[messages.length - 1] : null;
             var lastTimeCreated = mostRecentMessage ? mostRecentMessage.timeCreated : null;
 
-            if (lastTimeCreated && !isResetting && !isSendingMessage) {
+            if (lastTimeCreated && !isResetting && !isSendingMessage && !isDeletingConversationContent) {
                 // There may be multiple messages with the same time created value since
                 // the accuracy is only down to the second. The server will include these
                 // messages in the result (since it does a >= comparison on time from) so
@@ -893,6 +895,14 @@ function(
             }
         }
 
+        // Mark that we are deleting content from the  conversation to prevent updates of it.
+        isDeletingConversationContent = true;
+
+        // Stop polling for new messages to the open conversation.
+        if (newMessagesPollTimer) {
+            newMessagesPollTimer.stop();
+        }
+
         return deleteMessagesPromise.then(function() {
                 var newState = StateManager.removeMessagesById(viewState, messageIds);
                 newState = StateManager.removePendingDeleteMessagesById(newState, messageIds);
@@ -910,6 +920,7 @@ function(
                     PubSub.publish(MessageDrawerEvents.CONVERSATION_DELETED, newState.id);
                 }
 
+                isDeletingConversationContent = false;
                 return render(newState);
             })
             .catch(Notification.exception);
@@ -937,6 +948,14 @@ function(
         var newState = StateManager.setLoadingConfirmAction(viewState, true);
         render(newState);
 
+        // Mark that we are deleting the conversation to prevent updates of it.
+        isDeletingConversationContent = true;
+
+        // Stop polling for new messages to the open conversation.
+        if (newMessagesPollTimer) {
+            newMessagesPollTimer.stop();
+        }
+
         return Repository.deleteConversation(viewState.loggedInUserId, viewState.id)
             .then(function() {
                 var newState = StateManager.removeMessages(viewState, viewState.messages);
@@ -945,6 +964,8 @@ function(
                 newState = StateManager.setLoadingConfirmAction(newState, false);
                 PubSub.publish(MessageDrawerEvents.CONVERSATION_DELETED, newState.id);
 
+                isDeletingConversationContent = false;
+
                 return render(newState);
             });
     };
@@ -1132,10 +1153,11 @@ function(
                 return;
             })
             .catch(function(e) {
+                var errorMessage;
                 if (e.message) {
-                    var errorMessage =  $.Deferred().resolve(e.message).promise();
+                    errorMessage = $.Deferred().resolve(e.message).promise();
                 } else {
-                    var errorMessage =  Str.get_string('unknownerror', 'core');
+                    errorMessage = Str.get_string('unknownerror', 'core');
                 }
 
                 var handleFailedMessages = function(errorMessage) {
@@ -1211,8 +1233,6 @@ function(
 
     /**
      * Cancel edit mode (selecting the messages).
-     *
-     * @return {Promise} Renderer promise.
      */
     var cancelEditMode = function() {
         cancelRequest(getOtherUserId());
@@ -1249,6 +1269,8 @@ function(
                 renderable.deferred.resolve(true);
                 // Keep processing the buffer until it's empty.
                 processRenderBuffer(header, body, footer);
+
+                return;
             })
             .catch(function(error) {
                 isRendering = false;
@@ -1760,6 +1782,7 @@ function(
         renderBuffer = [];
         isResetting = true;
         isSendingMessage = false;
+        isDeletingConversationContent = false;
         sendMessageBuffer = [];
 
         var loggedInUserId = loggedInUserProfile.id;
index fed38be..4fa4b4a 100644 (file)
@@ -75,6 +75,7 @@ function(
 
     var LOAD_LIMIT = 50;
     var loadedConversationsById = {};
+    var deletedConversationsById = {};
     var loadedTotalCounts = false;
     var loadedUnreadCounts = false;
 
@@ -196,7 +197,6 @@ function(
     /**
      * Render the messages in the overview page.
      *
-     * @param {Object} contentContainer Conversations content container.
      * @param {Array} conversations List of conversations to render.
      * @param {Number} userId Logged in user id.
      * @return {Object} jQuery promise.
@@ -662,6 +662,7 @@ function(
                 return;
             }
 
+            var pendingPromise = new Pending('core_message/message_drawer_view_overview_section:new');
             var loggedInUserId = conversation.loggedInUserId;
             var conversationId = conversation.id;
             var element = getConversationElement(root, conversationId);
@@ -670,19 +671,34 @@ function(
                 var contentContainer = LazyLoadList.getContentContainer(root);
                 render([conversation], loggedInUserId)
                     .then(function(html) {
-                            contentContainer.prepend(html);
-                            element.remove();
-                            return html;
-                        })
+                        if (deletedConversationsById[conversationId]) {
+                            // This conversation was deleted at some point since the messaging drawer was created.
+                            if (conversation.messages[0].timeadded < deletedConversationsById[conversationId]) {
+                                // The 'new' message was added before the conversation was deleted.
+                                // This is probably stale data.
+                                return;
+                            }
+                        }
+                        contentContainer.prepend(html);
+                        element.remove();
+
+                        return;
+                    })
+                    .then(pendingPromise.resolve)
                     .catch(Notification.exception);
+            } else if (conversation.messages.length) {
+                createNewConversationFromEvent(root, conversation, loggedInUserId)
+                .then(pendingPromise.resolve)
+                .catch();
             } else {
-                createNewConversationFromEvent(root, conversation, loggedInUserId);
+                pendingPromise.resolve();
             }
         });
 
         PubSub.subscribe(MessageDrawerEvents.CONVERSATION_DELETED, function(conversationId) {
             var conversationElement = getConversationElement(root, conversationId);
             delete loadedConversationsById[conversationId];
+            deletedConversationsById[conversationId] = new Date();
             if (conversationElement.length) {
                 deleteConversation(root, conversationElement);
             }