Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(participants): cancel scheduled request when requesting new one #11064

Merged
merged 1 commit into from
Dec 3, 2023

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Dec 1, 2023

☑️ Resolves

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

image

🚧 Tasks

  • Test different scenarios

Patch for testing:

Index: src/mixins/getParticipants.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/mixins/getParticipants.js b/src/mixins/getParticipants.js
--- a/src/mixins/getParticipants.js	(revision 7cd9746d80542c1cc1b244c062fb4185aaded33a)
+++ b/src/mixins/getParticipants.js	(date 1701433028038)
@@ -35,6 +35,7 @@
 			participantsInitialised: false,
 			fetchingParticipants: false,
 			pendingChanges: false,
+			i: 0,
 			debounceFastUpdateParticipants: null,
 			debounceSlowUpdateParticipants: null,
 		}
@@ -48,12 +49,12 @@
 	},
 
 	created() {
-		this.debounceFastUpdateParticipants = debounce(function() {
-			this.cancelableGetParticipants()
+		this.debounceFastUpdateParticipants = debounce(function(i) {
+			this.cancelableGetParticipants(i)
 		}, 3000)
 
-		this.debounceSlowUpdateParticipants = debounce(function() {
-			this.cancelableGetParticipants()
+		this.debounceSlowUpdateParticipants = debounce(function(i) {
+			this.cancelableGetParticipants(i)
 		}, 15000)
 	},
 
@@ -98,6 +99,8 @@
 				return
 			}
 
+			this.i++
+			console.log('update', this.i)
 			// Clear previously requested updates
 			this.debounceFastUpdateParticipants.clear()
 			this.debounceSlowUpdateParticipants.clear()
@@ -105,18 +108,20 @@
 			// this.conversation is provided by component, where mixin is used
 			if (this.$store.getters.windowIsVisible()
 				&& (this.isInCall || !this.conversation?.hasCall)) {
-				this.debounceFastUpdateParticipants()
+				console.log('schedule Fast', this.i)
+				this.debounceFastUpdateParticipants(this.i)
 			} else {
-				this.debounceSlowUpdateParticipants()
+				console.log('schedule Slow', this.i)
+				this.debounceSlowUpdateParticipants(this.i)
 			}
 			this.pendingChanges = false
 		},
 
-		async cancelableGetParticipants() {
+		async cancelableGetParticipants(i) {
 			if (this.fetchingParticipants || this.token === '' || this.isInLobby || !this.isModeratorOrUser) {
 				return
 			}
-
+			console.log('request', i)
 			this.fetchingParticipants = true
 
 			const response = await this.$store.dispatch('fetchParticipants', { token: this.token })

🏁 Checklist

  • 🌏 Tested with Chrome, Firefox and Safari or should not be risky to browser differences
  • ⛑️ Tests are included or not possible

Copy link
Contributor

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the created() etc, so just to make sure because the title implies so:

When an update is scheduled, we should block scheduling new ones. If you cancel the schedule and schedule a new one, we will in worst case never update the list as there is a change all the time within the 15 seconds.

@Antreesy
Copy link
Contributor Author

Antreesy commented Dec 1, 2023

When an update is scheduled, we should block scheduling new ones

That's why debounce is used. But slow and fast debounced methods have different contexts, unfortunately

We will in worst case never update the list

Fair point. Then we could do it this way (dropping the parallel schedule only):

	if (this.$store.getters.windowIsVisible()
		&& (this.isInCall || !this.conversation?.hasCall)) {
		this.debounceSlowUpdateParticipants.clear()
		this.debounceFastUpdateParticipants()
	} else {
		this.debounceFastUpdateParticipants.clear()
		this.debounceSlowUpdateParticipants()
	}

Then:

  • slow -> fast -> fast -> ... fast will make a request in 3s
  • fast -> slow -> slow -> ... slow will make a request in 15s
  • slow -> fast -> slow -> ... fast -> slow ... will never make a request, if switch happens more frequent than 3s (but I can't see a case, where participant changing conditions so often)

@nickvergessen
Copy link
Member

(but I can't see a case, where participant changing conditions so often)

Webinar with 400 people... someone will always leave, someone will always join

@Antreesy
Copy link
Contributor Author

Antreesy commented Dec 1, 2023

someone will always leave, someone will always join

participant updates 'scheduling' depends only on yourself (this.$store.getters.windowIsVisible() && (this.isInCall || !this.conversation?.hasCall), so whatever 400 peoples are doing, you are always be either in slow or fast flow

P.S. there is also an option flush(), so if slow is scheduled, and we're calling fast, we can execute slow immediately, and then schedule fast in 3s. But library doesn't allow to track scheduled calls, and it still will be 2 requests instead of one

@DorraJaouad
Copy link
Contributor

there is also an option flush()

Yeah, this will only speed up the excecution but without blocking the second request

We will in worst case never update the list

aa right, I think it can be a non-edge case (user switching between tabs while in call multiple times).

PS: it's never too late for using ✨ flags ✨

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the fix/11048/clear-parallel-debounce-updates branch from 7cd9746 to 4ba284d Compare December 1, 2023 14:44
@Antreesy
Copy link
Contributor Author

Antreesy commented Dec 1, 2023

IMG_20231201_155021.jpg

Nailed it. Moved cancellation from debounceUpdateParticipants to cancelableGetParticipants

Explanation:

  • When we call 'slow' function at 00:00, it will be scheduled to execute in 00:15
  • If we call it again at 00:04, it overwrites first one, but will be scheduled to execute in 11s (same 00:15 from the first request)
  • If we call it again at 00:16, it schedules another request, but for 00:31

Same for calling 'fast', but as they have different context, they working in parallel.
If we cancel parallel request not when requesting a new one, but when new one is executed already, we will receive at least one response. So should be safe for both directions, as there can't be a loop anymore

@Antreesy
Copy link
Contributor Author

Antreesy commented Dec 3, 2023

/backport to stable28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

follow-up: slow and fast debounced methods are independent in getParticipants
3 participants