8000 Fix duplicated notifications on timeout by yurabakhtin · Pull Request #7519 · humhub/humhub · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix duplicated notifications on timeout #7519

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix duplicated notifications on timeout #7519

wants to merge 5 commits into from

Conversation

yurabakhtin
Copy link
Contributor

What kind of change does this PR introduce? (check at least one)

  • Bugfix

The PR fulfills these requirements:

  • All tests are passing
  • Changelog was modified

Other information:
humhub/calendar#547

@yurabakhtin yurabakhtin requested a review from luke- May 12, 2025 16:17
@yurabakhtin yurabakhtin mentioned this pull request May 12, 2025
Closed
@what-the-diff What The Diff
Copy link
what-the-diff bot commented May 12, 2025 •
edited
Loading

PR Summary

  • Included Fixes to the Change Log
    The update notes, or "change log", now includes information about a fix that was made to solve an issue with repetitive notifications that were being sent when the system didn't get a response in time.

  • Enhancements to Notification Management
    Improvements were made to the NotificationManager system which is in charge of sending messages. It now makes use of a new class, SendBulkNotification, to effectively send multiple notifications at once.

  • Modified sendBulk method
    The method in NotificationManager that was used to send bunch of notifications at once, named sendBulk, is now designed to take in a SendBulkNotification object, instead of individual notification and user query details. This alteration streamlines the message sending process.

  • Preventing Duplication with Cache
    New functions have been integrated into the SendBulkNotification class to prevent repeated notifications from being sent to users. Processed user IDs are conveniently kept in a quick-access storage space, known as a cache, to streamline this process.

  • Unique Identifier for SendBulkNotification
    A unique identification property has been added for each SendBulkNotification. This plays a key role in managing caches.

  • Retrieving Unprocessed Users
    A function has been created in SendBulkNotification that fetches user information while excluding those who have already received the notifications. With this function, we can ensure that notifications will reach each user only once, further enhancing the user experience.

@yurabakhtin yurabakhtin marked this pull request as draft May 12, 2025 20:08
8000
@yurabakhtin yurabakhtin marked this pull request as ready for review May 12, 2025 20:44
luke-
luke- requested changes May 13, 2025
View reviewed changes
Copy link
Contributor
@luke- luke- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yurabakhtin

  • The ActiveJob is currently set to no retry

    humhub/protected/humhub/modules/queue/LongRunningActiveJob.php

    Line 26 in dfa3f28

    return false;
    I find it strange that the timeout still triggers a retry. I think we should avoid that somehow?
  • Can you explain, why the retry is after 1 hour? Maybe the job runs about 1 hour before retry?
  • In principle, other queue drivers such as Redis should also be supported? I'm not sure if other queue features like “ExclusiveJob” work with it, but I think updating a task will be difficult to realize with other queue drivers.
  • I do like that we store the processed users, but a more lightweight solution would probably be nice here instead of serializing & save the whole object after each loop. Either cache or a lightweight DB table?

@yurabakhtin
Copy link
Contributor Author

@luke- Yes, it is very strange, it is why I could not reproduce the bug long time, and I found only this one way(set true there) to make the job repeatable.

  • Can you explain, why the retry is after 1 hour? Maybe the job runs about 1 hour before retry?

As I tested only with canRetry === true then the retry is fired after 1 hour because here $longRunningJobTtr = 60 * 60; (1 hour). I have only this explnation at this time, i.e. the failed job is repeated after 1 hour when the command yii queue/listen is running.

  • In principle, other queue drivers such as Redis should also be supported? I'm not sure if other queue features like “ExclusiveJob” work with it, but I think updating a task will be difficult to realize with other queue drivers.

I have implemented this only for queue drivers extended from database drivers yii\queue\db\Queue.
HumHub by default uses \humhub\modules\queue\driver\MySQL extended from yii\queue\db\Queue, but tests use humhub\modules\queue\driver\Instant extended from not db driver yii\queue\Queue, so there the fix is ignored.
I see Redis yii\queue\redis\Queue is extended from not db driver yii\queue\cli\Queue, then there it will not work. Should I investigate how it can be fixed for the driver?

  • I do like that we store the processed users, but a more lightweight solution would probably be nice here instead of serializing & save the whole object after each loop. Either cache or a lightweight DB table?

Hm, ok I will review again what we can do there, I thought it is not hard to update the queue record in db...

@yurabakhtin
Copy link
Contributor Author
  • I do like that we store the processed users, but a more lightweight solution would probably be nice here instead of serializing & save the whole object after each loop. Either cache or a lightweight DB table?

Hm, ok I will review again what we can do there, I thought it is not hard to update the queue record in db...

@luke- I have modified the code to store already processed user IDs in a cache - ebda853.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luke- luke- luke- requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yurabakhtin @luke-
0