-
Notifications
You must be signed in to change notification settings - Fork 376
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
Debounce stream management ack requests #1062
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I messed it up with queueMicroTask
sorry.
I'm ok with reintroducing this but I have a couple of questions
- Why 100ms ?
- What is the exact use case for this?
- Could it be covered by "hooking" to
sendMany
instead ?
Please add a test or we run into the risk of it breaking again
1. Why 100ms ?
It's honestly just my go-to subsecond value
2. What is the exact use case for this?
Avoiding sending r too often and keeping the network extra busy. Without
debounce I often see r being sent before we even heard from the last one
especially during login as a bunch of different iqs go out in a row.
3. Could it be covered by "hooking" to `sendMany` instead
I dont think this is related to sendMany, an application may have mani
different parts all sending stanzas out quickly at once that can't or aren't
done with sendMany, especially iqs.
…--
Stephen Paul Weber, @singpolyma
See <http://singpolyma.net> for how I prefer to be contacted
edition right joseph
|
It seems a bit low for this use case. Maybe 250ms? Any idea what others do? My intention with queueMicroTask was to send So this would send a single
but not
wdyt of this approach?
This could also happen with a 100ms debounce. Should we cancel the timeout and reschedule when receiving a
👍 |
> It's honestly just my go-to subsecond value
It seems a bit low for this use case. Maybe 250ms? Any idea what others do?
For sure, 250 could be a fine default.
My intention with queueMicroTask was to send `<r>` after each "tick"
So this would send a single `<r`:
```
xmpp.send(<foo/>)
xmpp.send(<foo/>)
```
but not
```
await xmpp.send(<foo/>)
await xmpp.send(<foo/>)
```
This is exactly what causes the stampede of overlapping ack requests during
any flurry of sends eg during first login.
|
How would you suggest to test something timing-dependent like this? |
4aa691d
to
0c79e92
Compare
Ok, figured out timer mocks and added a test |
So that they don't send after every stanza goes out but only when idle.
0c79e92
to
1283c54
Compare
So that they don't send after every stanza goes out but only when idle.