Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Useless pushes with no changes sent when reading rooms with no notifs enabled #11704

Closed
MatMaul opened this issue Jan 7, 2022 · 6 comments · Fixed by #11835
Closed

Useless pushes with no changes sent when reading rooms with no notifs enabled #11704

MatMaul opened this issue Jan 7, 2022 · 6 comments · Fixed by #11835
Labels
good first issue Good for newcomers S-Major Major functionality / product severely impaired, no satisfactory workaround. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@MatMaul
Copy link
Contributor

MatMaul commented Jan 7, 2022

Description

Duplicate pushes are sent whenever you read a room, even if notifications are disabled for this room and there is no change in the notifs count.

Steps to reproduce

  • read a room with no notifs (read receipt is sent)
  • a push is sent, identical to the previous push, with no event_id/room_id and an unchanged count

This is annoying for big accounts that are also extensively used on a computer device, because the phone basically never sleeps.

@reivilibre
Copy link
Contributor

I can see this being pretty bad because the excessive push traffic is bad for the app's 'reputation' essentially, on modern phone operating systems, resulting in pushes getting throttled.

A potential fix seems to be fairly simple:

  • update HttpPusher to add a field to keep track of the last sent badge count
  • in HttpPusher._update_badge, don't do anything if the count hasn't changed (and store the last-sent unread count)

@reivilibre reivilibre added good first issue Good for newcomers S-Major Major functionality / product severely impaired, no satisfactory workaround. T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. and removed T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements. labels Jan 7, 2022
@lukasdenk
Copy link
Contributor

I'll work on it.

@lukasdenk
Copy link
Contributor

@reivilibre Are you sure about your potential fix?
I have set a break point into the HttpPusher_update_badge and its callee, on_new_receipts. Everything worked but synapse never came to the breakpoints.

I tried the following setting:

  • setup a homeserver with default settings
  • started two Element clients (both on ubuntu)
  • created public rooms 1 and 2 with user A.
  • Joined rooms 1 and 2 with user B.
  • Send messages from user A to B and vice versa

@reivilibre
Copy link
Contributor

@lukasdenk I suppose I can't say I'm sure until it's actually proven.
However I think your problem is that your Element Web/Desktop clients won't be using a push gateway — you'd have to use a mobile client like Element Android/iOS or FluffyChat etc.
(It might be possible to use Web Push with the Hydrogen client which runs in a browser, but I haven't been keeping track so I'm not certain.)

If I was doing this, I would say that it would probably be easiest to first write a failing test for this situation described by MatMaul.
Now you can use breakpoints as you please, and you don't have to test it all manually with a real push gateway.
When you fix the issue, you even get a test included to ensure it keeps working afterwards! :)
(test_push_unread_count_message_count in tests.push.test_http is probably one of the closest tests you can refer to, for inspiration when writing your own test.)

@himanshi-2602
Copy link

@lukasdenk are you still working on this issue?
@reivilibre can I work on this issue?
I am a beginner but I am really interested in open source

@lukasdenk
Copy link
Contributor

@himanshi-2602 Yes, I am

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers S-Major Major functionality / product severely impaired, no satisfactory workaround. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants