Merge pull request #5795 from nextcloud/bugfix/5764/fix-concurrent-polling-cancel

Fix concurrency when cancelling lookForNewMessages

3 files changed, 64 insertions, 27 deletions

diff --git a/src/components/MessagesList/MessagesList.vue b/src/components/MessagesList/MessagesList.vue
index 50755d2d2..0173c90b2 100644
--- a/src/components/MessagesList/MessagesList.vue
+++ b/src/components/MessagesList/MessagesList.vue
@@ -79,6 +79,7 @@ import debounce from 'debounce'
 import { EventBus } from '../../services/EventBus'
 import LoadingPlaceholder from '../LoadingPlaceholder'
 import ChevronDown from 'vue-material-design-icons/ChevronDown'
+import { uniqueId } from 'lodash'
 
 export default {
 	name: 'MessagesList',
@@ -114,6 +115,8 @@ export default {
 
 	data() {
 		return {
+			viewId: null,
+
 			/**
 			 * When scrolling to the top of the div .scroller we start loading previous
 			 * messages. This boolean allows us to show/hide the loader.
@@ -236,7 +239,7 @@ export default {
 		},
 
 		chatIdentifier() {
-			return this.token + ':' + this.isParticipant + ':' + this.isInLobby
+			return this.token + ':' + this.isParticipant + ':' + this.isInLobby + ':' + this.viewId
 		},
 
 		scrollToBottomAriaLabel() {
@@ -259,13 +262,17 @@ export default {
 		},
 		chatIdentifier: {
 			immediate: true,
-			handler() {
+			handler(newValue, oldValue) {
+				if (oldValue) {
+					this.$store.dispatch('cancelLookForNewMessages', { requestId: oldValue })
+				}
 				this.handleStartGettingMessagesPreconditions()
 			},
 		},
 	},
 
 	mounted() {
+		this.viewId = uniqueId('messagesList')
 		this.scrollToBottom()
 		EventBus.$on('scrollChatToBottom', this.handleScrollChatToBottomEvent)
 		EventBus.$on('smoothScrollChatToBottom', this.smoothScrollToBottom)
@@ -283,7 +290,7 @@ export default {
 		EventBus.$off('focusMessage', this.focusMessage)
 		EventBus.$off('routeChange', this.onRouteChange)
 
-		this.$store.dispatch('cancelLookForNewMessages')
+		this.$store.dispatch('cancelLookForNewMessages', { requestId: this.chatIdentifier })
 		this.destroying = true
 
 		unsubscribe('networkOffline', this.handleNetworkOffline)
@@ -465,7 +472,7 @@ export default {
 					this.scrollToFocussedMessage()
 				})
 			} else {
-				this.$store.dispatch('cancelLookForNewMessages')
+				this.$store.dispatch('cancelLookForNewMessages', { requestId: this.chatIdentifier })
 			}
 		},
 
@@ -526,10 +533,12 @@ export default {
 
 			// Make the request
 			try {
+				// TODO: move polling logic to the store and also cancel timers on cancel
 				this.pollingErrorTimeout = 1
 				await this.$store.dispatch('lookForNewMessages', {
 					token: this.token,
 					lastKnownMessageId: this.$store.getters.getLastKnownMessageId(this.token),
+					requestId: this.chatIdentifier,
 				})
 
 				// Scroll to the last message if sticky
@@ -827,7 +836,7 @@ export default {
 		handleNetworkOffline() {
 			console.debug('Canceling message request as we are offline')
 			if (this.cancelLookForNewMessages) {
-				this.cancelLookForNewMessages()
+				this.$store.dispatch('cancelLookForNewMessages', { requestId: this.chatIdentifier })
 			}
 		},
 
diff --git a/src/store/messagesStore.js b/src/store/messagesStore.js
index 105ddf64a..0211da03c 100644
--- a/src/store/messagesStore.js
+++ b/src/store/messagesStore.js
@@ -103,7 +103,7 @@ const state = {
 	 * which allows to cancel the previous long polling request for new
 	 * messages before making another one.
 	 */
-	cancelLookForNewMessages: null,
+	cancelLookForNewMessages: {},
 	/**
 	 * Array of temporary message id to cancel function for the "postNewMessage" action
 	 */
@@ -191,8 +191,12 @@ const mutations = {
 		state.cancelFetchMessages = cancelFunction
 	},
 
-	setCancelLookForNewMessages(state, cancelFunction) {
-		state.cancelLookForNewMessages = cancelFunction
+	setCancelLookForNewMessages(state, { requestId, cancelFunction }) {
+		if (cancelFunction) {
+			Vue.set(state.cancelLookForNewMessages, requestId, cancelFunction)
+		} else {
+			Vue.delete(state.cancelLookForNewMessages, requestId)
+		}
 	},
 
 	setCancelPostNewMessage(state, { messageId, cancelFunction }) {
@@ -637,18 +641,21 @@ const actions = {
 	 *
 	 * @param {object} context default store context;
 	 * @param {string} token The conversation token;
-	 * @param {object} requestOptions reuquest options;
+	 * @param {string} requestId id to identify request uniquely
+	 * @param {object} requestOptions request options;
 	 * @param {int} lastKnownMessageId The id of the last message in the store.
 	 */
-	async lookForNewMessages(context, { token, lastKnownMessageId, requestOptions }) {
-		context.dispatch('cancelLookForNewMessages')
+	async lookForNewMessages(context, { token, lastKnownMessageId, requestId, requestOptions }) {
+		context.dispatch('cancelLookForNewMessages', { requestId })
 
 		// Get a new cancelable request function and cancel function pair
 		const { request, cancel } = CancelableRequest(lookForNewMessages)
+
 		// Assign the new cancel function to our data value
-		context.commit('setCancelLookForNewMessages', cancel)
+		context.commit('setCancelLookForNewMessages', { cancelFunction: cancel, requestId })
 
 		const response = await request({ token, lastKnownMessageId }, requestOptions)
+		context.commit('setCancelLookForNewMessages', { requestId })
 
 		if ('x-chat-last-common-read' in response.headers) {
 			const lastCommonReadMessage = parseInt(response.headers['x-chat-last-common-read'], 10)
@@ -722,12 +729,13 @@ const actions = {
 	 * Cancels a previously running "lookForNewMessages" action if applicable.
 	 *
 	 * @param {object} context default store context;
+	 * @param {string} requestId request id
 	 * @returns {bool} true if a request got cancelled, false otherwise
 	 */
-	cancelLookForNewMessages(context) {
-		if (context.state.cancelLookForNewMessages) {
-			context.state.cancelLookForNewMessages('canceled')
-			context.commit('setCancelLookForNewMessages', null)
+	cancelLookForNewMessages(context, { requestId }) {
+		if (context.state.cancelLookForNewMessages[requestId]) {
+			context.state.cancelLookForNewMessages[requestId]('canceled')
+			context.commit('setCancelLookForNewMessages', { requestId })
 			return true
 		}
 		return false
diff --git a/src/store/messagesStore.spec.js b/src/store/messagesStore.spec.js
index c5ab788c3..bd3e9dd35 100644
--- a/src/store/messagesStore.spec.js
+++ b/src/store/messagesStore.spec.js
@@ -869,7 +869,7 @@ describe('messagesStore', () => {
 		let updateConversationLastMessageAction
 		let updateUnreadMessagesMutation
 		let forceGuestNameAction
-		let cancelFunctionMock
+		let cancelFunctionMocks
 		let conversationMock
 
 		beforeEach(() => {
@@ -886,8 +886,10 @@ describe('messagesStore', () => {
 			testStoreConfig.actions.forceGuestName = forceGuestNameAction
 			testStoreConfig.mutations.updateUnreadMessages = updateUnreadMessagesMutation
 
-			cancelFunctionMock = jest.fn()
+			cancelFunctionMocks = []
 			CancelableRequest.mockImplementation((request) => {
+				const cancelFunctionMock = jest.fn()
+				cancelFunctionMocks.push(cancelFunctionMock)
 				return {
 					request,
 					cancel: cancelFunctionMock,
@@ -999,34 +1001,52 @@ describe('messagesStore', () => {
 		test('cancels look for new messages', async() => {
 			store.dispatch('lookForNewMessages', {
 				token: TOKEN,
+				requestId: 'request1',
 				lastKnownMessageId: 100,
 			}).catch(() => {})
 
-			expect(store.state.cancelLookForNewMessages).toBe(cancelFunctionMock)
-
-			expect(cancelFunctionMock).not.toHaveBeenCalled()
-
-			store.dispatch('cancelLookForNewMessages')
+			expect(cancelFunctionMocks[0]).not.toHaveBeenCalled()
 
-			expect(cancelFunctionMock).toHaveBeenCalledWith('canceled')
+			store.dispatch('cancelLookForNewMessages', { requestId: 'request1' })
 
-			expect(store.state.cancelLookForNewMessages).toBe(null)
+			expect(cancelFunctionMocks[0]).toHaveBeenCalledWith('canceled')
 		})
 
 		test('cancels look for new messages when called again', async() => {
 			store.dispatch('lookForNewMessages', {
 				token: TOKEN,
+				requestId: 'request1',
 				lastKnownMessageId: 100,
 			}).catch(() => {})
 
-			expect(store.state.cancelLookForNewMessages).toBe(cancelFunctionMock)
+			store.dispatch('lookForNewMessages', {
+				token: TOKEN,
+				requestId: 'request1',
+				lastKnownMessageId: 100,
+			}).catch(() => {})
+
+			expect(cancelFunctionMocks[0]).toHaveBeenCalledWith('canceled')
+		})
 
+		test('cancels look for new messages call individually', async() => {
 			store.dispatch('lookForNewMessages', {
 				token: TOKEN,
+				requestId: 'request1',
 				lastKnownMessageId: 100,
 			}).catch(() => {})
 
-			expect(cancelFunctionMock).toHaveBeenCalledWith('canceled')
+			store.dispatch('lookForNewMessages', {
+				token: TOKEN,
+				requestId: 'request2',
+				lastKnownMessageId: 100,
+			}).catch(() => {})
+
+			store.dispatch('cancelLookForNewMessages', { requestId: 'request1' })
+			expect(cancelFunctionMocks[0]).toHaveBeenCalledWith('canceled')
+			expect(cancelFunctionMocks[1]).not.toHaveBeenCalled()
+
+			store.dispatch('cancelLookForNewMessages', { requestId: 'request2' })
+			expect(cancelFunctionMocks[1]).toHaveBeenCalledWith('canceled')
 		})
 
 		describe('updates unread counters immediately', () => {