Fix concurrency when cancelling lookForNewMessages

When switching between regular mode and call mode, the two MessagesList
view instances are concurrently disturbing each other's
lookForNewMessages call.

To fix this, we now introduce a requestId for those requests to which we
pass in the chatIdentifier, extended by a generated view id. This brings
back the old behavior for now.

In the future we should look into keeping the polling request and
sharing it between the two Vues, which requires some refactoring of the
polling logic and timeouts.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>

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 })
 			}
 		},
 

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

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', () => {