Notifications.prune taking too long

[barisusakli]

This function needs rethinking, right now takes up to a minute to process causing the forum to become unresponsive.

Instead of running it on a cron for all users it can be run on a user basis when they receive a notification.

[julianlam]

Running it on a per-user basis would be fine, unless a user does not log in...

Notifications cron should only be run once a day anyway... when did it run for you?

[barisusakli]

Hmm I think its running 7pm est. Even with 650 users its taking 52 seconds, so it will be a huge problem on bigger forums.

Not really sure how the notification pruning works, I think it doesnt prune anything if they are not read? So if a user never logs in the notifications just keep increasing forever?

[julianlam]

Yes. I'll be the first to admit that notifications need an overhaul.

Notification pruning works thusly:

1. For every notification in the notifications set, check its expiry date
2. Also retrieve a copy of every user's inbox
3. If a notif has expired, and is not present in any user inboxi, delete it.

Steps 2 and 3 are obviously the O(derp) part, because the more users there are, the longer it will take. Step 3 is, I believe, running .every to ensure that the nid is not present in inboxes, but the inboxes array gets increasingly larger.

Instead, I think we should simpify the system:

• Broad delete all notifications older than x days (admin definable). Do not bother deleting it from user inboxes (uid:{uid}:notifications:unread)
• If a user requests a notification that has been deleted (nid-not-found), delete that reference from their inbox set.

[psychobunny]

Something I forgot to reply about much earlier:

Running it on a per-user basis would be fine, unless a user does not log in...

Not the case if so:

Instead of running it on a cron for all users it can be run on a user basis when they receive a notification.

[ThisIsMissEm]

The simplest possible solution would be to use a Child Process to do this work instead, hence not blocking the main web server thread.

[ThisIsMissEm]

That could be done with https://www.npmjs.org/package/cronos potentially.

[julianlam]

I believe the second task (in the ones I listed above) is already done, so I'll go ahead and just have prune delete the expired notification objects after 7 days.

Can extend this to be an admin definable value, but is not necessary at the moment.

[julianlam]

Hahaha, I like how the "status" for that module is "Alpha as f*ck". NodeBB already uses cron as a dependency, so no need to change, I think...