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

Sygnal sends all pushes with high priority level, which drains the devices resource #69

Closed
bmarty opened this issue Nov 29, 2019 · 6 comments

Comments

@bmarty
Copy link

bmarty commented Nov 29, 2019

Sygnal sends all pushes with high priority level (apns-priority 10 for iOs, and high for android), even for low priority events.

Priority of FCM push are explained here: https://firebase.google.com/docs/cloud-messaging/concept-options#setting-the-priority-of-a-message

This make the device to wake up, and the app to wake up and perform a sync.

On big accounts, it can be more than every 30 seconds, which make the Gplay app syncs more than the F-Droid version, which syncs every 30 seconds, when the app is in the background

In Android P, the system may ignore high priority Push for applications which are abusing high priority push (see https://developer.android.com/topic/performance/power/power-details)

The Android system may consider that the application is too battery greedy and will display some warning which is a bit annoying and deceptive for the user (along with the fact that the battery of its device is drained fast) (screenshot in French):

image

So we should use high priority Push for noisy event, and for event in e2e rooms maybe, and else use normal priority Push, which let the system choose when to deliver the Push to the app, regarding the current power mode of the device (screen on, idle, idle screen off not moving, etc.).

To fix that, Sygnal should know is the push to send has to be high or low priority, which may lead to change in the API.

Related issue:

@bmarty bmarty added the P1 label Nov 29, 2019
@erikjohnston
Copy link
Member

Looks like the push API already supports setting a priority on a notification (prio field). It defaults to high, so we should check that:

  1. Sygnal correctly sets the priority on FCM based on the field, and
  2. Ensure that Synapse is correctly sending that, including when its set to only send the event_id to Sygnal.

@reivilibre
Copy link
Contributor

Sygnal appears to correctly set the priority on both APNs and FCM pushes, but the spec says that a missing prio field should be treated as high priority, and Synapse doesn't appear to ever set the prio field [1].

@reivilibre reivilibre self-assigned this Jun 22, 2020
@erikjohnston
Copy link
Member

So. Having looked into this a bit (or should I say, had @reivilibre look into it 😀), it turns out that this is a bit of a can of worms, as: both Android and iOS want/require high priority push to trigger notifications on the device and not be ignored. Ideally this means that the server can figure out which events the client will display notifications for, and which they won't. This is complicated due to client side rules, such as decrypting e2e messages and then running the standard push rules on them.

(It's also worth noting that apps shouldn't be waiting for an API call to Synapse before displaying a notification, since e.g. a /sync can take a long time to return and the OS can kill the push handling thread before it returns. I think the suggestion in the past has been to wait a few seconds and if the API hit still hasn't returned to display a generic "You've received a new message" notification, or something like that).

This leads me to the conclusion that we can't easily fix this properly without a larger reworking of the notification and push rule system. As such I'm inclined to do something a bit hacky here that simply reduces the number of high priority pushes we send out, rather than ensuring that only push that trigger notifications will be high priority. I guess the simplest proposal would be to add some cheeky logic to Synapse so that only encrypted events and those with sound/highlight tweaks get given a high priority. (Unfortunately this can't happen on Sygnal due to it not having access to the event types and such).

I would be inclined not to spec this and simply say that this is Synapse specific logic, mainly as this feels like a hack and leads to behaviour which is not really correct. Ideally we'd follow up on this issue with a proper fix that we can then spec.

@reivilibre
Copy link
Contributor

matrix-org/synapse#7765 should solve this as a temporary solution

@reivilibre reivilibre removed their assignment Jul 16, 2020
@reivilibre
Copy link
Contributor

Should we now close this? This issue is a bit out of scope for Sygnal since it just does what it's told.

The hack we implemented is sad but realistic — anything more will need a rework of push rules (which is coming!) and perhaps broader push in general…

@richvdh
Copy link
Member

richvdh commented Sep 10, 2020

yeah, I think we can close this.

@richvdh richvdh closed this as completed Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants