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

Scheduled push notification is not sent immediately when user switches from online to away/offline #19097

Open
ceefour opened this issue Oct 1, 2020 · 7 comments · May be fixed by #19104
Open

Comments

@ceefour
Copy link
Contributor

ceefour commented Oct 1, 2020

Description:

New push notification scheduling behavior introduced by #17357 (@rodrigok) and #17907 (@sampaiodiego) is really awesome! But when user switches from (inaccurate) online to away/offline, push notification still waits for 120 seconds to expire. It needs to be sent immediately (from #17357 (comment)).

Steps to reproduce:

  1. Android app is online/foreground
  2. Kill the app or switch to another app
  3. Make sure that user's statusConnection is still online (this is another issue probably for Rocket.Chat.ReactNative how this sometimes can happen, but it does happen and quite easy to reproduce by just using the phone's task killer)
    • Reproducible easily using Samsung A50
    • I also tried using Realme X2 Pro and it's very hard to kill the app without making the app's status go offline. But this works: open Rocket.Chat for Android, turn on airplane mode, kill the app, turn off airplane mode, now Rocket.Chat server user's statusConnection is still online.
  4. Server has a posted message, and since user statusConnection is online according to server, meaning push is scheduled in 120 seconds
  5. Before message is received by app, and before 120 seconds timed out, user statusConnection is now either offline or away.

Expected behavior:

When a user's statusConnection becomes offline/away, server sends all delayed push notifications immediately.

Actual behavior:

Server still waits all delayed push notifications, even when a user's statusConnection becomes offline/away.

Server Setup Information:

  • Version of Rocket.Chat Server: 3.7.0
  • Operating System: Ubuntu 20.04.1
  • Deployment Method: GitHub 3.7.0 branch
  • Number of Running Instances: 1
  • DB Replicaset Oplog: local
  • NodeJS Version:
{
  'Rocket.Chat': '3.7.0',
  npm: '6.14.8',
  ares: '1.16.0',
  brotli: '1.0.7',
  cldr: '37.0',
  http_parser: '2.9.3',
  icu: '67.1',
  llhttp: '2.1.2',
  modules: '72',
  napi: '6',
  nghttp2: '1.41.0',
  node: '12.18.4',
  openssl: '1.1.1g',
  tz: '2019c',
  unicode: '13.0',
  uv: '1.38.0',
  v8: '7.8.279.23-node.39',
  zlib: '1.2.11'
}
  • MongoDB Version: 4.2.9

Client Setup Information

  • Desktop App or Browser Version: 4.10.0
  • Operating System: Android 10

Additional context

I'd like to contribute a fix, but I need some guidance of what files/functions are relevant

  1. when switching user's statusConnection
  2. how to call NotificationQueue so that all pending notifications for a user are sent immediately

Relevant logs:

@ceefour
Copy link
Contributor Author

ceefour commented Oct 1, 2020

I'm not sure who publishes this event but UserPresenceEvents.on('setUserStatus') listener in activeUsers.js is called, both:

  1. during regular Meteor call (logged as Meteor ➔ method UserPresence:online/away)
  2. without any Meteor call log, as offline, the status parameter is 0, but this is not logged at all (I'd add this logging at debug level, to make it easier to debug offline users and push notification issues).

@ceefour
Copy link
Contributor Author

ceefour commented Oct 1, 2020

My plan:

  • when activeUsers.js setUserStatus listener got status for away or offline, it will call NotificationClass to reset the scheduled time for all push notifications for that user (if any) to new Date()
    • somewhat the inverse effect of NotificationQueue.clearScheduleByUserId()
  • therefore, next time Notification.worker checks them, they will get sent immediately

@ceefour
Copy link
Contributor Author

ceefour commented Oct 1, 2020

I've made fix locally and tested it working. I'll submit a PR soon.

ceefour added a commit to ceefour/Rocket.Chat that referenced this issue Oct 1, 2020
ceefour added a commit to ceefour/Rocket.Chat that referenced this issue Oct 1, 2020
@ceefour
Copy link
Contributor Author

ceefour commented Oct 1, 2020

PR submitted as #19104, I hope @sampaiodiego can review and merge.

Thank you in advance for accepting my contribution. 🙏

ceefour added a commit to soluvas/Rocket.Chat that referenced this issue Oct 1, 2020
@sampaiodiego
Copy link
Member

hi @ceefour .. thanks a lot for your efforts and investigation, really appreciate that =)

the actual purpose of this "delay" before sending a notification was mainly to prevent people from receiving notification during a page reload or network instability (please correct me if I'm wrong @rodrigok )..

but I see the frustration it can cause when you're online go were away/offline "on purpose", which will cause the notifications to be delayed, but in this case you'd like to receive them right away..

thinking on this, maybe decreasing the delay would already be very beneficial.. it can already be controlled via an env var called NOTIFICATIONS_SCHEDULE_DELAY_ONLINE, which defaults to 120s. maybe something like 20s is enough to prevent the use case I described and improve over all experience.. what do you think? do you mind giving it a try?

@ceefour
Copy link
Contributor Author

ceefour commented Oct 6, 2020

@sampaiodiego my "on purpose" retaining online status is for reproducing purposes.

However, it is still possible that Rocket.Chat status will be "online" longer than it should, during typical usage. It has happened several times with several of our users, with varying Android devices.

I do feel 20 s is a better delay if this fix is rejected. But if this fix is applied, then a 120 s delay is acceptable.

I like this fix better because it's as close as possible to realtime, in any situation.

@eengstrom
Copy link

I'm glad to see lots of progress on this, and the original updates as part of #17357, but it's really unfortunate I had to find this bug report and related PRs to realize that the documentation is seriously out of date. Specifically:

Another unrelated but important configuration for push notification is user presence. So you know that when you are online on your web/desktop client, you won't receive push messages on your mobile.

If you are idle for 60 seconds, you will be considered away and all the mentions you have after 60 seconds will be pushed to your mobile client. But if you disable auto-away, you won't receive any push notifications unless your screen goes off due to your computer settings.

Depending on your settings per room or channel, you receive notifications on your mobile device for all messages or only if somebody mentions you. Notifications in rooms are only sent when the desktop client goes with no use for more than five minutes. Currently, changing your status to "Away" does not speed up the countdown.

Neither mentions 120s, nor the option to configure via ENV var.

Any chance to get those updated as well so others don't go through the effort I did in finding this information?

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