feat: debounce notifications #50
Conversation
link2xt
force-pushed
the
link2xt/debounce-notification
branch
from
738a5c8 to
1dce25a
Compare
| break; | ||
| }; | ||
|
|
||
| if now.duration_since(timestamp) < Duration::from_secs(1) { |
There was a problem hiding this comment.
So the debouncing timeout is just 1s? May it make sense to make it bigger to optimize more for multi-transport, but instead turn tokens into map storing also transport id (maybe some hash) and check it in notify(): if it's the same, then always notify and update heap? (need to count references in tokens then though)
This way if transports don't miss messages, we don't miss notifications. But if some transport is fast but not reliable, it may be worse.
There was a problem hiding this comment.
Yes, 1 second already debounces a lot of notifications. The problem it solves is that sometimes the same notification is stored twice for the same inbox, see the referenced issue.
if it's the same, then always notify
The most common case currently is that the user has one transport, and we don't want to notify twice if we get two notifications in a row for one transport.
There was a problem hiding this comment.
With the current 1s setting we already debounce 2k-10k per hour with 5-20 debounced tokens in the set at any time, so it is not a too small value.
link2xt
force-pushed
the
link2xt/debounce-notification
branch
from
197a07b to
e063bb3
Compare
link2xt
force-pushed
the
link2xt/debounce-notification
branch
from
e063bb3 to
522f385
Compare
link2xt
force-pushed
the
link2xt/debounce-notification
branch
from
522f385 to
bfd1c58
Compare
|
|
||
| info!("Got direct notification for {device_token}."); | ||
| let now = Instant::now(); | ||
| if state.debouncer().is_debounced(now, &device_token) { |
There was a problem hiding this comment.
To avoid double locking the debouncer mutex notify() could return whether the token is debounced
| //! In this case the same token is stored twice | ||
| //! for the same mailbox on a chatmail relay | ||
| //! and is notified twice for the same message. | ||
| //! Since it is not possible for the chatmail relay |
There was a problem hiding this comment.
Could the notification gateway tell the chatmail relay in the notification reply that the token was superseded? Otherwise this looks like we never clean up tokens on relays which can't work well on a large time scale
There was a problem hiding this comment.
If the token does not work anymore, "410 Gone" response is returned and the relay will remove the token immediately. Old tokens are also removed after 90 days of not being updated by the client, so a working duplicate will be removed eventually: chatmail/relay#583
|
|
||
| #[derive(Default)] | ||
| pub(crate) struct Debouncer { | ||
| state: Mutex<DebouncerState>, |
There was a problem hiding this comment.
It may make sense to turn it into RwLock because it's frequently locked just for reading
| fn cleanup(&mut self, now: Instant) { | ||
| loop { | ||
| let Some(Reverse((timestamp, token))) = self.heap.pop() else { | ||
| break; |
There was a problem hiding this comment.
There also can be debug_assert!(tokens.is_empty()); and even tokens.clear() just in case before break;
Close #49